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.

Sign up for GitHub

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

Jump to bottom

Add local version field in snapshots #294

Closed
wants to merge 7 commits into from
Closed

Add local version field in snapshots #294

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a0dacb4
Add local version field in snapshots
milanov Jan 9, 2016
4cec1b2
Switch the local version number to primitive long
milanov Jan 9, 2016
5fa19a7
Ensure backward-comparability by adding a version column to the alrea…
milanov Jan 11, 2016
fd8a2db
Change the way a column is checked for existence in the javers schema…
milanov Jan 11, 2016
94cbef6
Add separate tests for the local version serialization/deserialization
milanov Jan 12, 2016
29d7de6
added failing tests, bugs!
bartoszwalacik Jan 16, 2016
cfc154b
Fix deserializing object with a version from DB and start saving the …
milanov Jan 17, 2016
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
4 changes: 4 additions & 0 deletions ...rs-core/src/main/java/org/javers/core/json/typeadapter/commit/CdoSnapshotTypeAdapter.java
View file
Open in desktop
Expand Up @@ -26,6 +26,7 @@ class CdoSnapshotTypeAdapter extends JsonTypeAdapterTemplate<CdoSnapshot> {
static final String INITIAL_NAME_LEGACY = "initial";
static final String TYPE_NAME = "type";
static final String CHANGED_NAME = "changedProperties";
static final String VERSION = "version";

private TypeMapper typeMapper;

Expand All @@ -44,6 +45,7 @@ public CdoSnapshot fromJson(JsonElement json, JsonDeserializationContext context
JsonObject stateObject = (JsonObject)jsonObject.get(STATE_NAME);

GlobalId cdoId = context.deserialize(jsonObject.get(GLOBAL_CDO_ID), GlobalId.class);
Long version = context.deserialize(jsonObject.get(VERSION), Long.class);
DuckType duckType = new DuckType(cdoId.getTypeName(), extractPropertyNames(stateObject));

ManagedType managedType = typeMapper.getJaversManagedType(duckType, ManagedType.class);
Expand All @@ -57,6 +59,7 @@ public CdoSnapshot fromJson(JsonElement json, JsonDeserializationContext context

return builder
.withState(snapshotState)
.withVersion(version)
.withCommitMetadata(commitMetadata)
.withChangedProperties(changedProperties)
.build();
Expand Down Expand Up @@ -106,6 +109,7 @@ public JsonElement toJson(CdoSnapshot snapshot, JsonSerializationContext context
jsonObject.add(STATE_NAME, context.serialize(snapshot.getState()));
jsonObject.add(CHANGED_NAME, context.serialize(snapshot.getChanged()));
jsonObject.add(TYPE_NAME, context.serialize(snapshot.getType().name()));
jsonObject.add(VERSION, context.serialize(snapshot.getVersion()));

return jsonObject;
}
Expand Down
9 changes: 8 additions & 1 deletion javers-core/src/main/java/org/javers/core/metamodel/object/CdoSnapshot.java
View file
Open in desktop
Expand Up @@ -26,6 +26,7 @@ public final class CdoSnapshot extends Cdo {
private final CdoSnapshotState state;
private final SnapshotType type;
private final List<String> changed;
private final Long version;
Copy link
Member

Choose a reason for hiding this comment

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

Think we dont want version to be null, so maybe primitive long?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll change that. Do you prefer for me to simply commit this change or to squash it with the previous (I didn't find contribution guidelines)?

Copy link
Member

Choose a reason for hiding this comment

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

True, we need to write contribution guidelines. Just push another commit but wait a while, i will take a deeper look at this PR at the evening


/**
* should be assembled by {@link CdoSnapshotBuilder}
Expand All @@ -35,13 +36,15 @@ public final class CdoSnapshot extends Cdo {
CdoSnapshotState state,
SnapshotType type,
List<String> changed,
ManagedType managedType) {
ManagedType managedType,
Long version) {
super(globalId, managedType);
Validate.argumentsAreNotNull(state, commitMetadata, type, managedType);
this.state = state;
this.commitMetadata = commitMetadata;
this.type = type;
this.changed = changed;
this.version = version;
}

/**
Expand Down Expand Up @@ -121,4 +124,8 @@ public boolean isTerminal() {
public SnapshotType getType() {
return type;
}

public Long getVersion() {
return version;
}
}
14 changes: 12 additions & 2 deletions javers-core/src/main/java/org/javers/core/metamodel/object/CdoSnapshotBuilder.java
View file
Open in desktop
Expand Up @@ -26,6 +26,7 @@ public class CdoSnapshotBuilder {
private boolean markAllAsChanged;
private List<String> changed = Collections.EMPTY_LIST;
private ManagedType managedType;
private Long version;

private CdoSnapshotBuilder(GlobalId globalId, ManagedType managedType) {
Validate.argumentsAreNotNull(globalId, managedType);
Expand Down Expand Up @@ -57,20 +58,29 @@ public CdoSnapshotBuilder withState(CdoSnapshotState state) {
return this;
}

public CdoSnapshotBuilder withVersion(Long version) {
Validate.argumentIsNotNull(version);
this.version = version;
return this;
}

public CdoSnapshot build() {
if (state == null) {
state = stateBuilder.build();
}

if (previous != null){
if (previous != null) {
changed = state.differentValues(previous.getState());
version = previous.getVersion() + 1;
} else {
version = 1L;
}

if (markAllAsChanged){
changed = new ArrayList<>(state.getProperties());
}

return new CdoSnapshot(globalId, commitMetadata, state, type, changed, managedType);
return new CdoSnapshot(globalId, commitMetadata, state, type, changed, managedType, version);
}

public CdoSnapshotBuilder withType(SnapshotType type) {
Expand Down
9 changes: 7 additions & 2 deletions ...s-core/src/test/groovy/org/javers/core/json/typeadapter/CdoSnapshotTypeAdapterTest.groovy
View file
Open in desktop
Expand Up @@ -29,7 +29,6 @@ class CdoSnapshotTypeAdapterTest extends Specification {

when:
def jsonText = javers.jsonConverter.toJson(snapshot)
println jsonText

then:
def json = new JsonSlurper().parseText(jsonText)
Expand All @@ -41,6 +40,7 @@ class CdoSnapshotTypeAdapterTest extends Specification {
json.globalId.entity == "org.javers.core.model.DummyUser"
json.globalId.cdoId == "kaz"
json.type == "INITIAL"
json.version == 1
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to test it in a different (minimal) way.
So test new version field only in 2 basic tests:
def "should serialize CdoSnapshot to Json"() and
def "should deserialize CdoSnapshot"()
and not to change other tests.
Deserializer should not break if field is missing (and it will be missing if users ugrade to new JaVers version and will load snapshots created in old JaVers version).

}

def "should serialize state with primitive values in CdoSnapshot"() {
Expand Down Expand Up @@ -148,8 +148,8 @@ class CdoSnapshotTypeAdapterTest extends Specification {
state {
}
changedProperties changed
version "1"
}
println json.toString()

when:
def snapshot = javersTestAssembly().jsonConverter.fromJson(json.toString(), CdoSnapshot)
Expand All @@ -159,6 +159,7 @@ class CdoSnapshotTypeAdapterTest extends Specification {
snapshot.globalId == instanceId("kaz",DummyUser)
snapshot.initial == true
snapshot.changed.collect{it} as Set == ["name", "age"] as Set
snapshot.version == 1L
}

def "should deserialize CdoSnapshot state with primitive values"() {
Expand All @@ -181,6 +182,7 @@ class CdoSnapshotTypeAdapterTest extends Specification {
age 1
flag true
}
version "1"
}

when:
Expand Down Expand Up @@ -216,6 +218,7 @@ class CdoSnapshotTypeAdapterTest extends Specification {
cdoId 1
}
}
version "1"
}

when:
Expand Down Expand Up @@ -251,6 +254,7 @@ class CdoSnapshotTypeAdapterTest extends Specification {
}
id 1
}
version "1"
}

when:
Expand Down Expand Up @@ -289,6 +293,7 @@ class CdoSnapshotTypeAdapterTest extends Specification {
dateTimes "2000-01-01T12:00:00", "2000-01-01T12:00:00"
name "kaz"
}
version "1"
}

when:
Expand Down
23 changes: 23 additions & 0 deletions javers-core/src/test/groovy/org/javers/core/snapshot/SnapshotFactoryTest.groovy
View file
Open in desktop
Expand Up @@ -59,6 +59,29 @@ class SnapshotFactoryTest extends Specification{
updateSnapshot.changed == ["arrayOfIntegers"]
}

def "should update the version of the snapshot"() {
given:
def cdoWrapper = javers.createCdoWrapper(new SnapshotEntity(id: 1))
def prevSnapshot = snapshotFactory.createInitial(cdoWrapper, someCommitMetadata())

when:
def updateSnapshot = snapshotFactory.createUpdate(cdoWrapper, prevSnapshot, someCommitMetadata())

then:
updateSnapshot.version == 2L
}

def "should initialize the version of the first snapshot"() {
given:
def cdoWrapper = javers.createCdoWrapper(new SnapshotEntity(id: 1))

when:
def snapshot = snapshotFactory.createInitial(cdoWrapper, someCommitMetadata())

then:
snapshot.version == 1L
}

def "should mark changed and added properties of update snapshot"() {
given:
def ref = new SnapshotEntity(id:2)
Expand Down
2 changes: 2 additions & 0 deletions ...sistence-sql/src/main/java/org/javers/repository/sql/finders/CdoSnapshotObjectMapper.java
View file
Open in desktop
Expand Up @@ -27,6 +27,7 @@ class CdoSnapshotObjectMapper implements ObjectMapper<CdoSnapshot> {
private static final String STATE_NAME = "state";
private static final String TYPE_NAME = "type";
private static final String CHANGED_NAME = "changedProperties";
private static final String VERSION = "version";


public CdoSnapshotObjectMapper(JsonConverter jsonConverter, Optional<GlobalId> providedGlobalId) {
Expand All @@ -42,6 +43,7 @@ public CdoSnapshot createObject(ResultSet resultSet) throws SQLException {
json.add(STATE_NAME, jsonConverter.fromJsonToJsonElement(resultSet.getString(SNAPSHOT_STATE)));
json.add(CHANGED_NAME, assembleChangedPropNames(resultSet));
json.addProperty(TYPE_NAME, resultSet.getString(SNAPSHOT_TYPE));
json.addProperty(VERSION, resultSet.getLong(VERSION));

if (providedGlobalId.isPresent()){
json.add(GLOBAL_CDO_ID, jsonConverter.toJsonElement(providedGlobalId.get()));
Expand Down
1 change: 1 addition & 0 deletions javers-persistence-sql/src/main/java/org/javers/repository/sql/finders/SnapshotFilter.java
View file
Open in desktop
Expand Up @@ -21,6 +21,7 @@ abstract class SnapshotFilter {
static final String BASE_FIELDS =
SNAPSHOT_STATE + ", " +
SNAPSHOT_TYPE + ", " +
VERSION + ", " +
SNAPSHOT_CHANGED + ", " +
COMMIT_AUTHOR + ", " +
COMMIT_COMMIT_DATE + ", " +
Expand Down
2 changes: 2 additions & 0 deletions ...rs-persistence-sql/src/main/java/org/javers/repository/sql/schema/FixedSchemaFactory.java
View file
Open in desktop
Expand Up @@ -39,6 +39,7 @@ public class FixedSchemaFactory {
public static final String SNAPSHOT_COMMIT_FK = "commit_fk";
public static final String SNAPSHOT_GLOBAL_ID_FK = "global_id_fk";
public static final String SNAPSHOT_TYPE = "type";
public static final String VERSION = "version";
Copy link
Member

Choose a reason for hiding this comment

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

unfortunately, there is no db schema update in PolyJDBC (the tool we use to manage sql),
we do it manually, see ex JaversSchemaManager.alterCommitIdColumnIfNeeded()

So we need a new method which checks if column not exists and then runs DDL.
It's not easy to write and test.
There is another way we can try. Version field can be moved to CdoSnapshotState, which is serialized to JSON in all repositories. If so, no need to touch repositories code (only core JsonTypeAdapter).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I went this way but I'm a bit stuck on how to serialize the CdoSnapshotState including the new version field. Currently it is serialized as state: {prop1: val1, ..}. I can either add the version field inside of the state as another property (but then it could already have a property with the same name which means a conflict) or serialize it as state: {version: X, properties: {prop1: val1, ..}}, but this wouldn't be backward compatible. Any ideas on this?

Copy link
Member

Choose a reason for hiding this comment

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

You are right, there is no elegant way to serialize version in CdoSnapshotState. Looks like we need to come back to your original solution and write addColumnIfNotExists() method in JaversSchemaManager. We need to take a deep dive into PollyJDBC code. It's very lightweight but simple library ...

Copy link
Member

Choose a reason for hiding this comment

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

There are two possible solutions. We can write generic addColumnIfNotExists() method. Better place for this is PollyJDBC, but easier way is to write it in JaVers.
Second solution, would be quick hack, just a series of Ifs with proper DDLs,
like we did in line 57 in JaversSchemaManager

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There will be a problem with the previously stored snapshots, because they would have to all be traversed and assigned versions in incrementing order..This might not be that easy.

Copy link
Member

Choose a reason for hiding this comment

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

I think that one sub-select could be removed:

UPDATE jv_snapshot s SET s.version = (
SELECT COUNT(*) + 1 FROM jv_snapshot s2 WHERE s.global_id_fk = s2.global_id_fk and s2.snapshot_pk < s.snapshot_pk)

This query will be slow, but maybe good enough for migration script.

So PR is now ready to merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now it should be, I separated the test according what we discussed. As a note to the query from your last comment - I also tried it at first, but it gives an error in MySQL ("You can't specify target table 's' for update in FROM clause", as well as in PostgreSQL("ERROR: column "s" of relation "employees" does not exist") and I haven't tested it on the others.

Copy link
Member

Choose a reason for hiding this comment

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

ok, give us a few more days for merge and release

Copy link
Member

Choose a reason for hiding this comment

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

@milanov there are few bugs here:

  • NPE is thrown when version field is missing in a snapshot stored in SQL or Mongo (created by javers <= 1.4.2)
    migration script is optional, so JaVers cant crash when someone doesnt run it
    (I will fix it)
  • version is ALWAYS deserialized to 1
    take a look at line 60 in CdoSnapshotTypeAdapter. When snapshots are deserialized from DB, previous field is null,
    and builder overwrites number set in .withVersion(version)
  • version field is never write to Sql database, you simply forgot to add it to CdoSnapshotRepository.insertSnapshot()

Copy link
Member

Choose a reason for hiding this comment

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

see 12a8aff

public static final String SNAPSHOT_TABLE_PK_SEQ = "jv_snapshot_pk_seq";
public static final String SNAPSHOT_STATE = "state";
public static final String SNAPSHOT_CHANGED = "changed_properties"; //since v.1.2
Expand All @@ -48,6 +49,7 @@ private Schema snapshotTableSchema(Dialect dialect, String tableName){
RelationBuilder relationBuilder = schema.addRelation(tableName);
primaryKey(tableName, SNAPSHOT_PK, schema, relationBuilder);
relationBuilder.withAttribute().string(SNAPSHOT_TYPE).withMaxLength(200).and()
.withAttribute().longAttr(VERSION).and()
.withAttribute().text(SNAPSHOT_STATE).and()
.withAttribute().text(SNAPSHOT_CHANGED).and();
foreignKey(tableName, SNAPSHOT_GLOBAL_ID_FK, GLOBAL_ID_TABLE_NAME, GLOBAL_ID_PK, relationBuilder, schema);
Expand Down