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 19 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.
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.
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.
Does a global used-in make sense since Dataset does not need it? I think this should be specific for the entity.
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.
Here, I think it makes sense to keep it.
Since the disable is available for all entity types, so should used-in. The nice thing about it is that there can be a common approach to entities:
It is quite reassuring for the API user to be able to call
used-in
even if currently for a certain type there cannot be any entity linked, it perhaps may be in the future?All in all, I am a fan of having
used-in
s everywhere (where disable is).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.
From my point of view, this can stay
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.
Well, this is part of the v3 API outline. I consider this misleading, because since you have the option to export specific versions already ( at .../{model}/{name}/{versionEitherNumberOrLatestKeyword}/export, having the option for export here would have to have another meaning - e.g. export the whole entity with all the versions. But since this is designed as the latest export as well, I consider this unnecessary, and as I said, even misleading.
That is why I am proposing to omit this endpoint entirely.
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 think that if we gave the "latest" version we can omit this.
Another point to think about can be - export/import being on the entity itself. Export having parameters of name and 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.
After even further discussion, I have decided to remove the endpoint based on the following logic:
The structure of API endpoints representing a collection should, in general, behave in the following fashion:
GET /entityNameInPlural
-> list items or provide information about all/more itemsGET /entityNameInPlural/individualEntityKeyEgId
-> give information about a specific entityBy that logic,
GET /{name}/export
would be appropriate if all entities or information about all/more was returned -- perhaps all versions, whileGET /{name}/{version}/export
would export just a single version.But since we mapped just the single version (the latest one) export to a collection's endpoint, the logic does not fit. Moreover, given the fact that we also accept
latest
as a valid version identifier, it seem much more appropriate to useGET /{name}/{version}/export
here (specificallyGET /{name}/latest/export
)