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.

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

feat(api-v2): Return value UUID on value creation and update #1602

Merged
merged 8 commits into from Feb 27, 2020

Conversation

@benjamingeer
Copy link
Collaborator

@benjamingeer benjamingeer commented Feb 21, 2020

This PR makes API v2 return the UUID of a value when it is created or updated.

Resolves #1595.

@benjamingeer benjamingeer added the API/V2 label Feb 21, 2020
@benjamingeer benjamingeer added this to the 2020.1 milestone Feb 21, 2020
@benjamingeer benjamingeer self-assigned this Feb 21, 2020
@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 21, 2020

@tobiasschweizer This is a bit more work than I thought it would be, because currently all the UUID creation is done in Twirl templates.

@benjamingeer benjamingeer requested a review from tobiasschweizer Feb 21, 2020
@benjamingeer benjamingeer mentioned this pull request Feb 21, 2020
@tobiasschweizer
Copy link
Contributor

@tobiasschweizer tobiasschweizer commented Feb 24, 2020

This is a bit more work than I thought it would be, because currently all the UUID creation is done in Twirl templates.

It's not a problem if you need more time to work on this. So far, we still have a lot of work to do that does not depend on this PR.

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 24, 2020

It's not a problem if you need more time to work on this.

It's done now.

@tobiasschweizer
Copy link
Contributor

@tobiasschweizer tobiasschweizer commented Feb 24, 2020

Sorry, I kind of missed this. Thank you very much, I will do the review asap!

@tobiasschweizer
Copy link
Contributor

@tobiasschweizer tobiasschweizer commented Feb 24, 2020

I will also do a PR on knora-api-js-lib and change the message classes.

@tobiasschweizer
Copy link
Contributor

@tobiasschweizer tobiasschweizer commented Feb 24, 2020

Is there JSON test data for a successful response to a value creation / update?

@tobiasschweizer
Copy link
Contributor

@tobiasschweizer tobiasschweizer commented Feb 24, 2020

I integrated this PR here: dasch-swiss/dsp-js-lib#151

Copy link
Contributor

@tobiasschweizer tobiasschweizer left a comment

I wonder why you did not have to update JSON test data (adding UUID). Or is it that you don't use JSON test data because you can't know what a value's IRI and UUID are going to be?

@tobiasschweizer
Copy link
Contributor

@tobiasschweizer tobiasschweizer commented Feb 26, 2020

@benjamingeer Do you think you could create test data for the responses too (see #1602 (comment))?

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 26, 2020

@tobiasschweizer There's no need to ask me three times. :)

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 26, 2020

So far, the generation of test data doesn't actually update the repository. This is why there is no test response for creating a value. So I guess there are two options:

  1. Actually create a value and return the response. Then you have to be aware that your repository has been updated and other tests may not work.
  2. Return a simulated response. Then if the real response changes someday, the simulated response will have to change, too.

Which do you think is better?

@tobiasschweizer
Copy link
Contributor

@tobiasschweizer tobiasschweizer commented Feb 26, 2020

So far, the generation of test data doesn't actually update the repository. This is why there is no test response for creating a value. So I guess there are two options:

  1. Actually create a value and return the response. Then you have to be aware that your repository has been updated and other tests may not work.
  2. Return a simulated response. Then if the real response changes someday, the simulated response will have to change, too.

Which do you think is better?

I think a simulated response would do it. Could you generate the simulated response from an instance of a case class that is then serialized to JSON-LD? If the original case class changes or its serialization in JSON-LD, the simulated response will too.

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 26, 2020

Could you generate the simulated response from an instance of a case class that is then serialized to JSON-LD? If the original case class changes or its serialization in JSON-LD, the simulated response will too.

Good idea, I'll do that.

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 26, 2020

Could you generate the simulated response from an instance of a case class that is then serialized to JSON-LD?

Done in e3e1727.

@tobiasschweizer
Copy link
Contributor

@tobiasschweizer tobiasschweizer commented Feb 26, 2020

Thanks a lot for this! I will see if I can integrate this tomorrow, otherwise after my holiday.

@tobiasschweizer

This comment has been minimized.

Could you make this constant? stringFormatter.makeRandomResourceIri and stringFormatter.makeRandomValueIri return a new resource and value id whenever the test data is regenerated, e.g. on github-ci.

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 27, 2020

Could you make this constant?

Done in 673bff4.

@@ -127,7 +128,7 @@ INSERT {
knora-base:valueHasRefCount @linkUpdateForNewLink.newReferenceCount ;
knora-base:valueHasOrder ?order ;
knora-base:isDeleted false ;
knora-base:valueHasUUID "@{stringFormatter.base64EncodeUuid(UUID.randomUUID)}" ;
knora-base:valueHasUUID "@{stringFormatter.base64EncodeUuid(newLinkValueUUID)}" ;

This comment has been minimized.

@tobiasschweizer

tobiasschweizer Feb 27, 2020
Contributor

When the target of a link is changed, then the UUID is changed to? Is this because it has to be considered a new value (not a different version of the same value)?

This comment has been minimized.

@benjamingeer

benjamingeer Feb 27, 2020
Author Collaborator

That's right. This was a design decision that Lukas and I made a long time ago. We thought that if you change the rdf:object of a reification, it wouldn't make sense to consider it a new version of the same reification, because it no longer reifies the same triple. So when you change a link's target, the old LinkValue is deleted, and a new one is created.

This comment has been minimized.

@tobiasschweizer

tobiasschweizer Feb 27, 2020
Contributor

I figure when you just change the comment, the uuid stays the same, right?

This comment has been minimized.

@benjamingeer

benjamingeer Feb 27, 2020
Author Collaborator

Yes, if you just change the comment or any other metadata.

Copy link
Contributor

@tobiasschweizer tobiasschweizer left a comment

Thanks a lot for this PR! This should make the client's life easier.

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 27, 2020

Thanks for asking for this and doing the review!

@benjamingeer benjamingeer merged commit cbed601 into develop Feb 27, 2020
8 checks passed
8 checks passed
Build Everything
Details
API Unit Tests
Details
API E2E Tests
Details
API Integration Tests
Details
JS Lib Tests
Details
Upgrade Unit Tests
Details
Upgrade Integration Tests
Details
Docs Build Test
Details
@benjamingeer benjamingeer deleted the wip/1595-value-uuid branch Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

2 participants