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

fix for MySql INSERTS with autoincrement #32

Merged
merged 3 commits into from
Jun 12, 2017
Merged

Conversation

bartoszwalacik
Copy link
Contributor

No description provided.

@bgalek
Copy link

bgalek commented May 24, 2017

+1

Copy link
Member

@adamdubiel adamdubiel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think changes that you introduced change public APIs too much. From what i understand problem is that you shouldn't set 0 in autoincremented fields? Are all those changes necessary to achieve this?

*/
public boolean isSequenceValueSet() {
return sequenceValueSet;
void setArgument(String fieldName, Object value) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InsertQuery was supposed to have fluent API, adding this delegator breaks everything - why can't it be done gracefully?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chyba nie rozumiem, nie dodawałem żadnych delegatorów. Co dokładnie 'breaks everything'?

Jedyna zmiana w API to usunięcie możliwości ustawiania ręcznie ID, więc 2 metod: sequenceValue(long value) i isSequenceValueSet(), ponieważ nie byłem w stanie ich zachować po wydzieleniu klasy InsertWithAutoincrement.

Te metody są a) niepotrzebne, b) nie mają sensu dla MySql, btw kto chciałby robić coś takiego ?

InsertQuery query = javersPolyJDBC.query().insert().into("my_table")              
                .value("my_field", 1)
                .sequence("my_id", "ANYTHING !")
                .sequenceValue(5);

Jeśli nie chcę używać sekwencji to po prostu ich nie używam i ustawiam ID wprost:

InsertQuery query = javersPolyJDBC.query().insert().into("my_table")              
                .value("my_field", 1)
                .value("my_id", "5");

@Override
public long generateKey(String sequenceName, Transaction transaction) throws SQLException {
return DEFAULT_VALUE;
throw new RuntimeException("not implemented");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you expand this message a bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will change to throw new RuntimeException("Not implemented. Can't generate key on AutoIncremented");

/**
* Manually set sequenced field value.
*/
public InsertQuery sequenceValue(long value) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you removed public method - this is a breaking change, is it neccessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this method is used by polyjdbc clients.
Typically you run queries via polyjdbc QueryRunner:

        InsertQuery query = javersPolyJDBC.query().insert().into(getSnapshotTableNameWithSchema())
                .value(SNAPSHOT_TYPE, cdoSnapshot.getType().toString())
                .value(SNAPSHOT_GLOBAL_ID_FK, globalIdPk)
                .value(SNAPSHOT_COMMIT_FK, commitIdPk)
                .value(SNAPSHOT_VERSION, cdoSnapshot.getVersion())
                .value(SNAPSHOT_STATE, jsonConverter.toJson(cdoSnapshot.getState()))
                .value(SNAPSHOT_CHANGED, jsonConverter.toJson(cdoSnapshot.getChanged()))
                .value(SNAPSHOT_MANAGED_TYPE, cdoSnapshot.getManagedType().getName())
                .sequence(SNAPSHOT_PK, getSnapshotTablePkSeqWithSchema());

        return javersPolyJDBC.queryRunner().insert(query);

and QueryRunner increments the sequence and saves the last value. Why to bypass the flow?

@@ -73,23 +71,11 @@ public InsertQuery into(String tableName) {
* Insert next sequence value into column of given name. Only one sequenced
* column per table is supported so far.
*/
public InsertQuery sequence(String sequenceField, String sequenceName) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after these changes API will look quite ugly:

InsertQuery query = QueryFactory.instert();
((InsertWithAutoIncrement) query()).sequence(.....)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, this API is not changed, the sequence() method is still in the same place :)

@adamdubiel adamdubiel merged commit e3a6034 into master Jun 12, 2017
@adamdubiel adamdubiel deleted the InsertWithAutoincrement branch June 12, 2017 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants