Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feature/1693 api v3 versioned model - Dataset v3 implementation #2046
Feature/1693 api v3 versioned model - Dataset v3 implementation #2046
Changes from 27 commits
f1798b2
0f62851
c01cfda
4fd08e2
5c8e005
458bfed
a7a6cf9
af489b1
21f8008
6f63117
76e87ce
d62cd90
fc4c359
e6c8d8c
3566df5
219360a
112ac1e
101b37a
b934958
51ad013
ba5edce
662119b
d480445
3542a43
d6ef401
a75ab85
9241eb1
09c326e
1c241d1
5d24819
7040694
e0fda73
a80d9ac
562da60
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't
Range
be used forversions
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, it could. Or even
VersionedSummary
with just the latest version would do (assuming that all entities start with v1).Attachments are the exception, they take after the schema version and there may be holes, but attachments are not exposed directly.
Then there is the matter of not being able to prove that all versions are actually there in the DB, we assume they should be, but... how sure can you be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps let's just use just VersionedSummary - with the latest version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the V2 and V3 suffixes are because of Spring autowiring?
(Looks ugly to me, to be honest 😄 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this has nothing to do with spring autowiring. This is just a way to have V2 and V3 implementation in some services so that the v2 version can be removed easily when only v3 is to stay.
If we want to keep v2 and v3 long-term, this would require to create separate services, e.g.
datasetService
anddatasetServiceV3
. One of these solutions is, I believe, necessary if you want to keep the original implementation working.It then begs the question - what is the intended lifetime of V2 API? Knowing this better, it could help to design better: copy-paste-and-change-a-few-things knowing that the V2 impl will be gone soon vs. keeping v3 extending v2 (no code duplication, but harder removal)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The v3 method moved to DatasetServiceV3, so now it is tidier. Thanks for the tip, @benedeki.