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

Add local version field in snapshots #294

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@milanov
Contributor

milanov commented Jan 9, 2016

I'm happy to accept any comments, suggestions, etc :)

@@ -26,6 +26,7 @@
private final CdoSnapshotState state;
private final SnapshotType type;
private final List<String> changed;
private final Long version;

This comment has been minimized.

@bartoszwalacik

bartoszwalacik Jan 9, 2016

Member

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

This comment has been minimized.

@milanov

milanov Jan 9, 2016

Contributor

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)?

This comment has been minimized.

@bartoszwalacik

bartoszwalacik Jan 9, 2016

Member

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

@bartoszwalacik bartoszwalacik self-assigned this Jan 9, 2016

@@ -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

This comment has been minimized.

@bartoszwalacik

bartoszwalacik Jan 9, 2016

Member

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).

@@ -39,6 +39,7 @@
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";

This comment has been minimized.

@bartoszwalacik

bartoszwalacik Jan 9, 2016

Member

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).

This comment has been minimized.

@milanov

milanov Jan 9, 2016

Contributor

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?

This comment has been minimized.

@bartoszwalacik

bartoszwalacik Jan 10, 2016

Member

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 ...

This comment has been minimized.

@bartoszwalacik

bartoszwalacik Jan 10, 2016

Member

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

This comment has been minimized.

@milanov

milanov Jan 11, 2016

Contributor

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.

This comment has been minimized.

@bartoszwalacik

bartoszwalacik Jan 12, 2016

Member

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?

This comment has been minimized.

@milanov

milanov Jan 12, 2016

Contributor

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.

This comment has been minimized.

@bartoszwalacik

bartoszwalacik Jan 13, 2016

Member

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

This comment has been minimized.

@bartoszwalacik

bartoszwalacik Jan 16, 2016

Member

@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()

This comment has been minimized.

@bartoszwalacik

milanov added some commits Jan 11, 2016

@bartoszwalacik bartoszwalacik added documentation and removed review labels Jan 19, 2016

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jan 19, 2016

I did fixes on my branch, and just merged all into master

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jan 20, 2016

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