Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#186 [RFC][WIP] Sequence Support #88

Closed
6 of 12 tasks
michalbundyra opened this issue Jan 16, 2020 · 8 comments
Closed
6 of 12 tasks

#186 [RFC][WIP] Sequence Support #88

michalbundyra opened this issue Jan 16, 2020 · 8 comments

Comments

@michalbundyra
Copy link
Member

For original thought process on this list see #186

  • Explicit sequence with string or array identifier
  • Multiple sequences per table (would love ideas for improvement)
  • Implicit sequence with column name only for SERIAL pseudo-type
    • Generate based on PostgreSQL concatenation rule
    • Query database metadata if identifier ends up being longer than 63 bytes
  • Add SERIAL pseudo-type to column creation DDL to compliment idea of implicit sequence at DB creation/migrations level.
  • Update Insert and Update statements to filter out or generate values based on the changes
  • Consider making SequenceFeature independent from TableGateway since (1) it does not necesseirly tie to a specific table, (2) fix RowGateway limitation
  • Update Metadata TableGateway feature to recognize SERIAL columns and add SequenceFeature instances
  • Add MS SQL Server 2016 support
  • Stretch goal: add Oracle 12c implicit sequence support

Update 24th December 2016:

Supersedes, combines PR #162 and #172. Addresses issues brought up by @andrey-mokhov about implicit sequences. To avoid extra query, first constructing name using same naming rule as PostgreSQL and when on rare occasions exceed length, query database. Improve on original solution by using prepared statement in order to cache queries instead of recompiling what is a very common statement with only identifier name variation.
Combines with @xorock solution of specifying schema together with sequence name. Parametrized statement should also deal with error prone manual value escaping.

FeatureSet filter

First, need to adjust FeatureSet to handle more than one instance of a type to categorize callMagicCall() properly.

This should help with the fact that even though TableGateway accepts unrestricted array of features, if more than one feature of the same type is added, only first one will be processed. Useful for sequences or anyone else who may have run into this.

What I am not fully happy with, could use verification on before I go too far into this:

  1. Features of same type now need to have a filter of the target method to ensure they are indeed the target recipient. https://github.com/zendframework/zend-db/compare/master...alextech:sequences?expand=1#diff-85fe938f5a6f87443ad2578472eed9e0R112
    This is needed so can have $tableGateway->lastSequenceId('implicit_by_column_name'); or $tableGateway->lastSequenceId('explicit_sequence_name'); calls to be delegated to appropriate sequence instance out of the array of them. Not the cleanest looking solution.

  2. Is categorization by class name the best way to go? Is https://github.com/zendframework/zend-db/compare/master...alextech:sequences?expand=1#diff-a46ec23198cadca611ac2ff22cbe055dR156 the best way to loop through them? Maybe to address first point should use combination of feature class name and some unique identifier like name. But then not all features will have an obvious identifier.


Originally posted by @alextech at zendframework/zend-db#187

@michalbundyra
Copy link
Member Author

Unfortunately, I've to close this PR due to inactivity.


Originally posted by @ezimuel at zendframework/zend-db#187 (comment)

@michalbundyra
Copy link
Member Author

@ezimuel I was waiting on some assistance about difficult architecture decisions outlined in last comment at zendframework/zend-db#186 and a potential discrepancy with another PR linked in todo list. I got stuck not knowing how to go about those conflicts, what more experienced maintainers would do in that case, but never got reply :(


Originally posted by @alextech at zendframework/zend-db#187 (comment)

@michalbundyra
Copy link
Member Author

@ezimuel
We should reopen this PR, because this PR is still work in progress and @alextech does a good job here. Only some feedback is needed! Maybe you can review this PR.


Originally posted by @froschdesign at zendframework/zend-db#187 (comment)

@michalbundyra
Copy link
Member Author

@ezimuel
Inactivity lies on our side!

@alextech
Would you complete this PR?


Originally posted by @froschdesign at zendframework/zend-db#187 (comment)

@michalbundyra
Copy link
Member Author

@alextech ok, I'm re-opening this PR. I'll try to schedule some time for review it. It's a lot of code :(


Originally posted by @ezimuel at zendframework/zend-db#187 (comment)

@michalbundyra
Copy link
Member Author

@froschdesign I will put the effort in! Thank you! It'll be challenging to rethink something this complicated a year later so I will take some days to catch up to my older self, but an opportunity to make Zend actually work I do not want to miss. If this PR is closed, then several bug reports and PRs from others about sequences will have to be ignored: they are very deep rabbit holes.

@ezimuel It is a lot of code indeed :( For now, would you be able to at least go through conversations I had linked from #186? Especially the choice to be made in last comment of that issue, while keeping in mind last conversation at #162 where I mention that PDO driver solves some of the problems out of the box. (as an extra, may glance through #177)

Also, if I can assume #233 will be merged in, that will help, because it resolves quoting duplication logic.


Originally posted by @alextech at zendframework/zend-db#187 (comment)

@michalbundyra
Copy link
Member Author

@ezimuel Moving some of current questions I have from original bug report here so its easier for you to follow. Thank you for the blog post! Was motivational. Not pressuring you to answer these right away

Original code had switch/case statement per platform. Thats what bug reports focused on so I followed same pattern: https://github.com/zendframework/zend-db/pull/187/files#diff-85fe938f5a6f87443ad2578472eed9e0R166

But I just noticed there is duplication of that lastSequenceId() query in Connection class. Seems like a great benefit because do not need nasty switch($platform), and if anyone relied on similarly broken code in Connection too, all is fixed in one place.

https://github.com/zendframework/zend-db/blob/master/src/Adapter/Driver/Pgsql/Connection.php#L267

https://github.com/zendframework/zend-db/blob/master/src/Adapter/Driver/Pdo/Connection.php#L419

There are some concerns, however. What about nextSequenceId()? Connection interface does not have signature for it the way getLastGeneratedValue() is and rightfully so, I believe, should not be added. I could add nextSequenceId() to connection classes without adding to interface and assume dynamic typing will handle it, but that looks dirty. But code duplication looks dirty too, and so does inconsistency of doing switch/case for one, and call to adapter for the other method.

Another possible downside of using adapter connection, is getLastGeneratedValue() does not share same syntax for SqlServer 2016 where its one command for IDENTITY column but a different one for SEQUENCE object/column. (not a problem for postgresql or oracle up to 12c because sequence is the only way to do it)

Finally, how do I go about deducing implicit sequence names which too is a platform specific action? My getSequenceName() is awefully postgresql specific. I am not sure if decorator is possible to do for TableGateway features.


Originally posted by @alextech at zendframework/zend-db#187 (comment)

@weierophinney
Copy link
Member

This package is considered feature-complete, and is now in security-only maintenance mode, following a decision by the Technical Steering Committee.
If you have a security issue, please follow our security reporting guidelines.
If you wish to take on the role of maintainer, please nominate yourself

If you are looking for an actively maintained package alternative, we recommend:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants