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
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 1 addition & 4 deletions src/main/java/org/polyjdbc/core/key/AutoIncremented.java
Expand Up @@ -25,12 +25,9 @@
* @author Adam Dubiel
*/
public class AutoIncremented implements KeyGenerator {

private static final long DEFAULT_VALUE = 0;

@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");

}

@Override
Expand Down
49 changes: 11 additions & 38 deletions src/main/java/org/polyjdbc/core/query/InsertQuery.java
Expand Up @@ -15,9 +15,13 @@
*/
package org.polyjdbc.core.query;

import org.polyjdbc.core.key.KeyGenerator;
import org.polyjdbc.core.transaction.Transaction;
import org.polyjdbc.core.type.ColumnTypeMapper;
import org.polyjdbc.core.util.StringBuilderUtil;

import java.sql.SQLException;

/**
* Builds insert query, use {@link QueryFactory#insert() } to create new instance.
*
Expand All @@ -30,7 +34,7 @@
*
* @author Adam Dubiel
*/
public class InsertQuery {
public abstract class InsertQuery {

private static final int VALUES_LENGTH = 50;

Expand All @@ -40,12 +44,6 @@ public class InsertQuery {

private final StringBuilder values = new StringBuilder(VALUES_LENGTH);

private String sequenceField;

private String sequenceName;

private boolean sequenceValueSet;

InsertQuery(ColumnTypeMapper typeMapper) {
this.query = new Query(typeMapper);
}
Expand Down Expand Up @@ -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 :)

this.sequenceField = sequenceField;
this.sequenceName = sequenceName;
return value(sequenceField, sequenceField);
}
public abstract InsertQuery sequence(String sequenceField, String sequenceName);

boolean sequenceSet() {
return sequenceName != null;
}

String getSequenceName() {
return sequenceName;
}
abstract boolean isIdInserted();

String getSequenceField() {
return sequenceField;
}
abstract long generateSequenceValue(KeyGenerator keyGenerator, Transaction transaction) throws SQLException;

/**
* Insert value into column of given name. Object is automatically translated
Expand All @@ -100,24 +86,11 @@ String getSequenceField() {
public InsertQuery value(String fieldName, Object value) {
valueNames.append(fieldName).append(", ");
values.append(":").append(fieldName).append(", ");
query.setArgument(fieldName, value);
setArgument(fieldName, value);
return this;
}

/**
* 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?

query.setArgument(sequenceField, value);
sequenceValueSet = true;
return this;
}

/**
* Returns true if sequenced field value was set manually and there is no
* need to use sequence generator.
*/
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");

query.setArgument(fieldName, value);
}
}
32 changes: 32 additions & 0 deletions src/main/java/org/polyjdbc/core/query/InsertWithAutoincrement.java
@@ -0,0 +1,32 @@
package org.polyjdbc.core.query;

import org.polyjdbc.core.key.KeyGenerator;
import org.polyjdbc.core.transaction.Transaction;
import org.polyjdbc.core.type.ColumnTypeMapper;

import java.sql.SQLException;

public class InsertWithAutoincrement extends InsertQuery {

public InsertWithAutoincrement(ColumnTypeMapper typeMapper) {
super(typeMapper);
}
private boolean isIdInserted = false;

@Override
public InsertQuery sequence(String sequenceField, String sequenceName) {
//pretend that DB has sequences as we don't want to complicate clients' code
isIdInserted = true;
return this;
}

@Override
boolean isIdInserted() {
return isIdInserted;
}

@Override
long generateSequenceValue(KeyGenerator keyGenerator, Transaction transaction) throws SQLException {
throw new RuntimeException("not implemented");
}
}
35 changes: 35 additions & 0 deletions src/main/java/org/polyjdbc/core/query/InsertWithSequence.java
@@ -0,0 +1,35 @@
package org.polyjdbc.core.query;

import org.polyjdbc.core.key.KeyGenerator;
import org.polyjdbc.core.transaction.Transaction;
import org.polyjdbc.core.type.ColumnTypeMapper;

import java.sql.SQLException;

class InsertWithSequence extends InsertQuery {
private String sequenceField;
private String sequenceName;

InsertWithSequence(ColumnTypeMapper typeMapper) {
super(typeMapper);
}

@Override
public InsertQuery sequence(String sequenceField, String sequenceName) {
this.sequenceField = sequenceField;
this.sequenceName = sequenceName;
return value(sequenceField, sequenceField);
}

@Override
boolean isIdInserted() {
return sequenceName != null;
}

@Override
long generateSequenceValue(KeyGenerator keyGenerator, Transaction transaction) throws SQLException {
long key = keyGenerator.generateKey(sequenceName, transaction);
setArgument(sequenceField, key);
return key;
}
}
5 changes: 4 additions & 1 deletion src/main/java/org/polyjdbc/core/query/QueryFactory.java
Expand Up @@ -33,7 +33,10 @@ public QueryFactory(Dialect dialect, ColumnTypeMapper typeMapper) {
* Create insert query.
*/
public InsertQuery insert() {
return new InsertQuery(typeMapper);
if (dialect.supportsSequences()) {
return new InsertWithSequence(typeMapper);
}
return new InsertWithAutoincrement(typeMapper);
}

/**
Expand Down
Expand Up @@ -102,19 +102,22 @@ public boolean queryExistence(SelectQuery query) {
@Override
public long insert(InsertQuery insertQuery) {
try {
boolean useSequence = insertQuery.sequenceSet();
long key = 0;

if (useSequence) {
long key = keyGenerator.generateKey(insertQuery.getSequenceName(), transaction);
insertQuery.sequenceValue(key);
if (insertQuery.isIdInserted() && insertQuery instanceof InsertWithSequence) {
key = insertQuery.generateSequenceValue(keyGenerator, transaction);
}

Query rawQuery = insertQuery.build();
try(PreparedStatement statement = rawQuery.createStatementWithValues(transaction)) {
transaction.executeUpdate(statement);
}

return useSequence ? keyGenerator.getKeyFromLastInsert(transaction) : 0;
if (insertQuery.isIdInserted() && insertQuery instanceof InsertWithAutoincrement) {
key = keyGenerator.getKeyFromLastInsert(transaction);
}

return key;
} catch (SQLException exception) {
transaction.rollback();
Query rawQuery = insertQuery.build();
Expand Down
@@ -0,0 +1,38 @@
package org.polyjdbc.core.key

import org.polyjdbc.core.PolyJDBC
import spock.lang.Shared
import spock.lang.Specification

import static org.polyjdbc.core.key.PolyFactory.insertAndSelectOneRecord

class MySqlAutoincrementInsertTest extends Specification {
@Shared PolyJDBC polyJDBC

def setupSpec() {
this.polyJDBC = createPoly()

if (!polyJDBC.schemaInspector().relationExists('test')) {
PolyFactory.createSchema(polyJDBC)
}
}

private PolyJDBC createPoly() {
PolyFactory.newPolyForMySql()
}

def "should insert records with autoincremented id"(){
given:
def recordsToInsert = 100
def inserted = 0

when:
recordsToInsert.times {
def selected = insertAndSelectOneRecord(it, this.polyJDBC)
if (selected == it) inserted ++
}

then:
inserted == recordsToInsert
}
}
62 changes: 62 additions & 0 deletions src/test/groovy/org/polyjdbc/core/key/PolyFactory.groovy
@@ -0,0 +1,62 @@
package org.polyjdbc.core.key

import org.polyjdbc.core.PolyJDBC
import org.polyjdbc.core.PolyJDBCBuilder
import org.polyjdbc.core.dialect.DialectRegistry
import org.polyjdbc.core.infrastructure.DataSourceFactory
import org.polyjdbc.core.query.mapper.ObjectMapper
import org.polyjdbc.core.schema.model.Schema

import java.sql.ResultSet
import java.sql.SQLException

class PolyFactory {

static PolyJDBC newPolyForPostgres() {
def dialect = DialectRegistry.POSTGRES.dialect
def dataSource = DataSourceFactory.create(dialect, 'jdbc:postgresql://localhost:5432/polly', 'javers', 'javers')

PolyJDBCBuilder.polyJDBC(dialect, null).connectingToDataSource(dataSource).build()
}

static PolyJDBC newPolyForMySql() {
def dialect = DialectRegistry.MYSQL.dialect
def dataSource = DataSourceFactory.create(dialect, 'jdbc:mysql://localhost/polly', 'javers', '')

PolyJDBCBuilder.polyJDBC(dialect, null).connectingToDataSource(dataSource).build()
}

static void createSchema(PolyJDBC polyJDBC) {
def schema = new Schema(polyJDBC.dialect(), null);

schema.addRelation("test")
.withAttribute().longAttr("id").withAdditionalModifiers("AUTO_INCREMENT").and()
.withAttribute().integer("some_count").and()
.constrainedBy().primaryKey("pk_test").using("id").and()
.build()
schema.addSequence("seq_test").build()

def manager = polyJDBC.schemaManager()
manager.create(schema)
polyJDBC.close(manager)
}

static int insertAndSelectOneRecord(long value, PolyJDBC polly) {
def insertQuery = polly.query().insert().into("test").sequence("id", "seq_test")
.value("some_count", value)
def queryRunner = polly.queryRunner()

long insertedId = queryRunner.insert(insertQuery)

def selectQuery = polly.query().select("some_count").from("test").where("id = $insertedId");
def selected = queryRunner.queryUnique(selectQuery, new ObjectMapper() {
Object createObject(ResultSet resultSet) throws SQLException {
resultSet.getLong("some_count")
}
})
println "inserted $value at $insertedId, selected: $selected, thread: " + Thread.currentThread().name
queryRunner.commitAndClose()

selected
}
}