Skip to content
This repository was archived by the owner on Mar 11, 2022. It is now read-only.

Type of sinceSeq can be also a String besides an Integer#527

Merged
mojito317 merged 16 commits intomasterfrom
526-sinceseq-should-be-a-string
Jul 7, 2021
Merged

Type of sinceSeq can be also a String besides an Integer#527
mojito317 merged 16 commits intomasterfrom
526-sinceseq-should-be-a-string

Conversation

@mojito317
Copy link
Copy Markdown
Contributor

@mojito317 mojito317 commented Jul 5, 2021

Checklist

  • Tick to sign-off your agreement to the Developer Certificate of Origin (DCO) 1.1
  • Added tests for code changes or test/build only changes
  • Updated the change log file (CHANGES.md|CHANGELOG.md) or test/build only changes
  • Completed the PR template below:

Description

Fixes #526

Approach

seqs could be Integers in CouchDB 1.x and Strings in 2.x, so I changed all the places where they remained Integers to JsonElement. With this, both Integer and String can be used.

Security and Privacy

  • No change

Testing

  • This behavior can be tested with ITs well, so I just extended ITs with a plus .sinceSeq(...) call which is going to use a seq from a {source_db}/_changes request.

Monitoring and Logging

  • No change

@mojito317 mojito317 changed the title Change type of sinceseq from Integer to String Change type of sinceseq from Integer to JsonElement Jul 5, 2021
@mojito317 mojito317 marked this pull request as ready for review July 5, 2021 15:59
@mojito317 mojito317 requested a review from ricellis July 6, 2021 08:03
Copy link
Copy Markdown
Member

@ricellis ricellis left a comment

Choose a reason for hiding this comment

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

Nearly there, as discussed offline I think the seq things should preferably be in their own tests.
Otherwise 2 other things:

  • don't change the existing API in the ReplicatorDocument model class
  • changes entry

}

public Integer getSinceSeq() {
public JsonElement getSinceSeq() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To avoid breaking compatibility with anyone using this still with e.g. Couch 1.x we should not change this API.
I suggest reverting this and adding:
String getSinceSeqString() - which can output the JsonElement string (for both string and number seq types), the existing int method can be modified to check if the JsonElement is a number primitive and return the int if it is and throw (I guess an IllegalStateException) if it isn't.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 103eb3b and 0c4e3c8

@mojito317 mojito317 changed the title Change type of sinceseq from Integer to JsonElement Type of sinceSeq can be also a String besides an Integer Jul 6, 2021
@mojito317
Copy link
Copy Markdown
Contributor Author

Nearly there, as discussed offline I think the seq things should preferably be in their own tests.

Done in 79abd33. Lmk if you would like to see more.

don't change the existing API in the ReplicatorDocument model class

Done in 103eb3b and 0c4e3c8.

  • changes entry

Done in 64770f4.

@mojito317 mojito317 requested a review from ricellis July 6, 2021 12:45
Copy link
Copy Markdown
Member

@ricellis ricellis left a comment

Choose a reason for hiding this comment

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

+1 just a couple nits

Comment thread CHANGES.md Outdated
@mojito317
Copy link
Copy Markdown
Contributor Author

+1 just a couple nits

Thanks for the thorough review! I fixed the nits in f3018dd.

Copy link
Copy Markdown
Contributor

@emlaver emlaver left a comment

Choose a reason for hiding this comment

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

Looks good! We should consider adding a negative test to assert that an error message is thrown when passing in an unexpected type.

@mojito317
Copy link
Copy Markdown
Contributor Author

We should consider adding a negative test to assert that an error message is thrown when passing in an unexpected type.

An unexpected sinceSeq (e.g. a "true" or a "asd") will cause a crashing state in the replication and the replication will fail and timeout eventually in the waitForReplicatorToReachStatus method:

throw new TimeoutException("Timed out waiting for replication to complete");

Meanwhile, the replication document will disappear from the _replicator database.

Other types cannot be added, because compilation would fail.

@mojito317
Copy link
Copy Markdown
Contributor Author

Thanks for the reviews @ricellis and @emlaver! I'm about to merge this!

@mojito317 mojito317 merged commit 107c28d into master Jul 7, 2021
@mojito317 mojito317 deleted the 526-sinceseq-should-be-a-string branch July 7, 2021 14:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SinceSeq should be a string

3 participants