Add $this->select to the output of getRawState() in Sql/Insert.php (Issue 175)

[dmalouf]

Per Issue #175, this PR simply adds $this->select to the output of getRawState() (or getRawState('select')). This is parallel to the way Select::getRawState() returns values, objects, or null - specifically, the way Select returns objects for: having, table, where.

That is, getRawState() does not have to only return scalars, arrays, or null - it can also return objects. Therefore, this PR does not have getRawState('select') return the getRawState() of that select value (that is, this PR $insert->getRawState('select') is not the same as $insert->getRawState($select->getRawState())).

[alextech]

Better function name. There are already SELECT related tests in the file. I only know what this does vs others based on our conversation, not the name. Maybe, testSelectIncludedInRawState

[dmalouf]

Yup - your suggested function name is way better! Making the change.

[alextech]

Columns should be populated based on what $this->select has inside of it. Reason is this SequenceFeature https://github.com/zendframework/zend-db/blob/master/src/TableGateway/Feature/SequenceFeature.php#L52. It looks at the insert statement and compares its column names with one marked as primary key and has sequence attached to it. If it does, it will replace value of what is attempted to be inserted manually, with auto-generated sequence value. A convenient safety net against database query crashes.

Currently, it does not seem to take care of column marked as SERIAL (as pointed out by #172, which I am still yet to combine with #162 as I volunteered, plus document it for #165 ), but either way these 3 issues/PRs will depend on clean handling of columns populated by sequences, which means it needs to be conflict free regardless of whether it is coming from manual $insert->columns(....) or automated $insert->($select). Since that conflict resolution is done through getRawState, 'columns' is out to be filled properly.

This leads to a more complex problem. How would the value be replaced in SELECT the way https://github.com/zendframework/zend-db/blob/master/src/TableGateway/Feature/SequenceFeature.php#L63 is doing now if such conflict arrises.

1. For starters, those with SELECT * (no columns specified), can throw exception saying that insertion into sequenced table from "select *" is too risky for data consistency.
2. Mentioned PRs need more refactoring so that nextSequenceID does not have both SQL for next SEQUENCE generated and gives back value. Will need extra function that can be rused for generating that NEXTVAL thing for both grabbing next ID whenever user wants as right now, and to be passed in as subquery to $this->select replacing value of whatever column name = PK name and/or sequenced name (not to forget that table may have multiple sequenced columns that will conflict with passed SELECT, some implicit, some explicit). Select object should with some effort be modifiable with an expression in place of original value dynamically, I hope.
3. Additional column names from joins need somehow weeding out too for same purpose.

But that is beyond the scope of this PR. Just something that will need to be done to make those 2 PRs plus the #165 docs describing this mess possible, where knowing affected columns will help. I guess this invalidates my concern about having 'select' as an object instead of array, because having it as a referenced object, means I can "hack" at its values to resolve the sequence conflict from within SequenceFeature, which I would not have been able to if it remained as a protected property.

[dmalouf]

This is really fascinating stuff - way beyond my current comprehension and use of zend-db!!

May I be so bold as to ask: Does simply adding 'select' to the output of getRawState() prevent the work being-done / done-on #162 and #172 from working correctly? I'd obviously hate to break other code, new issues being tackled, etc. I'm concerned that my lack of understanding of what you are writing in this comment is keeping me from seeing how adding this key-value pair to getRawState() is breaking other issues/code.

Or when you write, "But that is beyond the scope of this PR" - does that mean that your comment above is for documentation that can be used in #162 and #172, and that there is nothing in the code of this PR that needs to change?

[alextech]

No, it does not prevent it from working correctly. It actually helps.
This PR will not break any existing code.
However, I can see yet another bug report in sequence feature area in few weeks/month time with someone claiming a bug with a usecase I posted in the issue discussion.

I tend to get too verbose and sometimes my point get lost in the wall of text (to which this sentence ironically contributes) so will try to highlight the issue again.

To re-emphasize ,your code so far does not break anything, but is incomplete, if filling out columns key is left out. It can technically be accepted, but I personally do not feel it will be appropriate since that sequence thing is another bug report just waiting to happen and it can be prevented if this PR is expanded on to allow for a facility to fully inspect column values with SELECT the same way it is done with manual column specification.

Generally, if we are fixing the raw state of Insert class for usecase of Select object, will help if do it all the way, not half. So far, adding select to rawstate alone does not give full picture of the state which makes this fix slighly misleading - users of the class will think it does. Users of sequence thing will expect that API works the same way, manual table or select. Having columns sometimes be part of the state and sometimes not, will cause problems, is inconsistent, since this is what state inspectors rely on. Yes, they can perhaps watch out for presence of 'select' key, but still need a reliable mechanism to read out Insert's columns in all conditions, and anywhere else it may be needed across the framekwork or 3rd party libs, we may not realize, not just in sequences. Not something I feel is appropriate to be left out. If the rawState is expected to give full report of whats about to happen to the database, it needs to be reliable in both cases.

It is an interesting problem I will find some free time to assist with, if that proves to be difficult. Now, if you feel that is something too complex for this and should be a separate PR, that is maybe a valid concern. But then we have a problem of multiple PRs from different places depend on one another before they are even merged, so rather fix the issue fully at once. If you do not agree with that, I can work on separate PR that supersedes this one eventually with an overall fix.

==done editing, sorry==

[dmalouf]

@alextech - I'm slowly understanding more of what you're addressing. Since I never do anything with Sequence stuff, I'm confident I do not fully grasp the situation fully at all (ha).

If I may offer a different perspective on multiple PRs for this Insert.php class: putting multiple Issues' needs into one PR can also be confusing 'down the road.' If the value of the 'columns' key in getRawState() needs to have logic in it that is needed in the Sequence class(es), then it might be better to put such changes into the PR that uses such code changes.

I FULLY understand the dilemma of "PRs from different places depend on one another..." What I'm not understanding (and this could very well have to do with my not knowing a thing about the Sequence stuff you're talking about) is how adding 'select' to getRawState() has any affect on Sequence other than actually making the Select object available? If anything, it seems that adding 'select' makes this whole Insert object more inspectable while keeping the Select object (and its columns) unique.

Perhaps you can point me to another zend-db class where a non-essential object's properties (values, column-names, something else) are 'added' to the essential properties of the containing class? The other, main class that we use at work is the Select class which does not pull data from classes it 'contains' (ex. Expression objects, TableIdentifier object, etc.) but instead simply returns the object-itself for the user to parse as-desired.

Or to look at it another way, if we change the content of getRawState('columns') in this PR, then would that not be a breaking change whereas adding the ability to inspect the Select object - even though this introduces more conditionals in situations like the Sequence stuff you mention - at least gives access to what is needed without breaking how the current method/contract works?

Again, if I am fully misunderstanding the situation you are describing, please just let me know and perhaps we can figure out a solution that accomplishes what you're seeing but - due to my lack of experience - doesn't require me to learn everything about the Sequence problems (grin).

[alextech]

If anything, it seems that adding 'select' makes this whole Insert object more inspectable while keeping the Select object (and its columns) unique.
Yes, this is a major improvement already. Many thanks for that :D :D

So part of me does agree that we can stop right here, and make an emphasis in documentation that whenever $insert->select() is used then getRawSate will not have 'columns' filled in and it will be responsibility of the API user to check for existence of 'select' key. If you go this route, I would recommend an appropriate documentation blob is added to this PR (https://github.com/zendframework/zend-db/blob/master/doc/book/sql.md#insert section) that usage of $insert->select() will make 'columns' not filled in and it will be up to the user to inspect select as you suggest for any potential conflicts manually.

Cant think of a good class to show at the moment :(

how adding 'select' to getRawState() has any affect on Sequence other than actually making the Select object available
This comes from API definition. Yes it may be obvious if someone reads through the source code of Zend DB prior to using it and realized that "oh, this code skips population of 'columns' in such and such conditions. Ok, I better accommodate for that". But then how many people read the source prior to usage? Then we have complains either ZF docs are incomplete or too hard to use because a developer who just wants to use the class will hit ctrl+space in editor, pick getRawState(), point it to 'columns' array and then what will he/she think when it is semi-randomly empty? Yet another bug report here. The effect it has on sequence, or any 3rd party lib inspecting the state, is something that would be covered by abstraction layer is not there. Which leads to answer of

If we change the content of getRawState('columns') in this PR, then would that not be a breaking change whereas adding the ability to inspect the Select object - even though this introduces more conditionals in situations like the Sequence stuff you mention - at least gives access to what is needed without breaking how the current method/contract works.
If developers expect abstraction layer to do a job, in this case automatically supply list of columns that will be affected (or else whats the point of abstraction layer if I have to do things manually), then a case where that promise is broken and i have to fall back to manual inspections, that is breaking change to me. If there is a common way to inspect Select object for columns, it should be included as a utility, as opposed to leaving it up to developers reinvent this part every time they call getRawState() (plus reading the source to understand why it does not follow the expectations).

I do not think you are misunderstanding as much as you say you do. (and this is not just about sequences anymore, that is just the first place I found a problem in, but API design in general) You are not giving yourself enough credit. You have good respectable arguments :) Hope I am not coming too strong against it and we are still on good discussion terms - you are giving me great thinking exercise :)

I feel we came to a standstill, both having valid arguments, and since I am not the framework's maintainer, I have no further say in right or wrong way. Will have to call to @weierophinney to be the judge.

[dmalouf]

Thank you for this great dialog, @alextech! You are definitely not "coming too strong against" my thoughts on these topics - I have learned quite a bit plus you have given me a different perspective on writing APIs. Very good stuff!

As I am new to being involved with large, public projects like ZF2, I was unsure what we do 'next' in our dialog. I am glad you suggested we let @weierophinney be the judge - good idea!

Until the next PR :-)

[alextech]

Good addition! Still not convinced, however, it should not be 'select'=> $this->select->getRawState() but not for me to decide. Looking at other classes, yes, they do use a mix of object vs string but to me it looks like negligence not implementing it as array everywhere. But maybe official maintainers know something I do not. Since no apparent rule, I guess this is fine. (convinced myself against it being a problem based on SequenceFeature complexity thoughts)

[michalbundyra]

This repository has been moved to laminas/laminas-db. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

• Squash all commits in your branch (git rebase -i origin/{branch})
• Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
• Run the laminas/laminas-migration tool on the code.
• Clone laminas/laminas-db to another directory.
• Copy the files from the second bullet point to the clone of laminas/laminas-db.
• In your clone of laminas/laminas-db, commit the files, push to your fork, and open the new PR.
  We will be providing tooling via laminas/laminas-migration soon to help automate the process.

[michalbundyra]

This repository has been closed and moved to laminas/laminas-db; a new issue has been opened at laminas/laminas-db#92.