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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 8 commits into from
Feb 27, 2020
Merged

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

merged 8 commits into from
Feb 27, 2020

Conversation

benjamingeer
Copy link

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

Resolves #1595.

@benjamingeer benjamingeer added this to the 2020.1 milestone Feb 21, 2020
@benjamingeer benjamingeer self-assigned this Feb 21, 2020
@benjamingeer
Copy link
Author

@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 mentioned this pull request Feb 21, 2020
Open
@tobiasschweizer
Copy link
Contributor

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
Author

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

It's done now.

@tobiasschweizer
Copy link
Contributor

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

@tobiasschweizer
Copy link
Contributor

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

@tobiasschweizer
Copy link
Contributor

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

@tobiasschweizer tobiasschweizer mentioned this pull request Feb 24, 2020
Merged
@tobiasschweizer
Copy link
Contributor

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

tobiasschweizer
tobiasschweizer reviewed Feb 24, 2020
View reviewed changes
Copy link
Contributor

@tobiasschweizer tobiasschweizer left a comment

Choose a reason for hiding this comment

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

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

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

@benjamingeer
Copy link
Author

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

@benjamingeer
Copy link
Author

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

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
Author

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
Author

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

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

@benjamingeer
Copy link
Author

Could you make this constant?

Done in 673bff4.

tobiasschweizer
tobiasschweizer reviewed Feb 27, 2020
View reviewed changes
webapi/src/main/twirl/queries/sparql/v2/changeLinkTarget.scala.txt
@@ -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)}" ;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

tobiasschweizer
tobiasschweizer approved these changes Feb 27, 2020
View reviewed changes
Copy link
Contributor

@tobiasschweizer tobiasschweizer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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

@benjamingeer
Copy link
Author

Thanks for asking for this and doing the review!

@benjamingeer benjamingeer merged commit cbed601 into develop Feb 27, 2020
@benjamingeer benjamingeer deleted the wip/1595-value-uuid branch February 27, 2020 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobiasschweizer tobiasschweizer tobiasschweizer approved these changes

Assignees

@benjamingeer benjamingeer

Labels
API/V2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Obtaining UUID on Value Creation
2 participants
@benjamingeer @tobiasschweizer