feat(api-v2): Make values citable #1322
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
benjamingeer
changed the title
added 8 commits
May 20, 2019 17:46
# Conflicts: # webapi/src/main/scala/org/knora/webapi/responders/v2/ResourcesResponderV2.scala # webapi/src/main/scala/org/knora/webapi/responders/v2/SearchResponderV2.scala # webapi/src/main/scala/org/knora/webapi/responders/v2/StandoffResponderV2.scala # webapi/src/main/scala/org/knora/webapi/util/ConstructResponseUtilV2.scala # webapi/src/test/resources/test-data/ontologyR2RV2/knoraApiOntologyWithValueObjects.rdf
benjamingeer
pushed a commit
to dasch-swiss/ark-resolver
that referenced
this pull request
May 22, 2019
|
A question: Do you plan to use the text value's uuid in the standoff route too? If so, how would Knora guarantee that the client gets all the standoff belonging to one specific version of a text value? |
I was thinking that we shouldn't use the UUID in the standoff route, exactly for this reason. |
benjamingeer
pushed a commit
to dasch-swiss/ark-resolver-data
that referenced
this pull request
May 22, 2019
benjamingeer
changed the title
|
This pull request has been mentioned on Discuss DaSCH. There might be relevant details there: |
added 6 commits
May 27, 2019 15:03
# Conflicts: # webapi/src/test/resources/test-data/ontologyR2RV2/knoraApiOntologyWithValueObjects.rdf
|
This pull request has been mentioned on Discuss DaSCH. There might be relevant details there: |
https://discuss.dasch.swiss/t/fine-grained-citations/24/4?u=benjamingeer |
| @@ -170,6 +170,45 @@ class ValuesRouteV2E2ESpec extends E2ESpec { | |||
| } | |||
|
|
|||
| "The values v2 endpoint" should { | |||
| "get the latest version of a value, given its UUID" in { | |||
There was a problem hiding this comment.
Are these the same tests as for ResourcesResponderV2Spec, just for the values route this time?
There was a problem hiding this comment.
I don't understand the question. The resources route doesn't accept value UUIDs, and this PR doesn't add anything to ResourcesResponderV2Spec.
There was a problem hiding this comment.
I think it's just that the resources responder is used internally to get specific values so the tests were added to ResourcesResponderV2Spec. But the values route v2 is called in the API so the tests go into ValuesRouteV2E2ESpec.
There was a problem hiding this comment.
Sorry, I got confused. Yes, that's right.
There was a problem hiding this comment.
no problem, I first had to understand that v1 and v2 handling of values is different :-)
In that case we need their feedback on this PR. |
I don't see what I could possibly change to make this easier for them. Adding more features to Knora usually means adding more information to API responses. We can't stop development on Knora. |
| @@ -83,6 +83,9 @@ DELETE { | |||
|
|
|||
| *@ | |||
| ?resource <@{linkUpdate.linkPropertyIri}Value> ?linkValue@linkValueIndex . | |||
|
|
|||
| @* Delete the UUID from the current version of the link value, because the new version will store it. *@ | |||
There was a problem hiding this comment.
if this creates a new value, why would there already be a UUID?
There was a problem hiding this comment.
If we're creating a new text value, we may have to update existing link values for standoff links. If there are existing link values, they will have UUIDs.
There was a problem hiding this comment.
Ah, I remember! There is only one link but the ref count could be more than one
There was a problem hiding this comment.
posttraumatic standoff disorder -> PTSD
There was a problem hiding this comment.
It was not meant as a joke. I am actually suffering from it ;-)
|
|
||
| *@ | ||
| @if(linkUpdate.linkValueExists) { | ||
| <@linkUpdate.newLinkValueIri> knora-base:previousValue ?linkValue@linkValueIndex . | ||
| <@linkUpdate.newLinkValueIri> knora-base:previousValue ?linkValue@linkValueIndex ; |
There was a problem hiding this comment.
Wouldn't you have to remove the UUID from the previous version?
There was a problem hiding this comment.
Yes, that happens here:
I want to add another test to make sure these SPARQL updates insert and delete the UUIDs correctly.
There was a problem hiding this comment.
I want to add another test to make sure these SPARQL updates insert and delete the UUIDs correctly.
On this PR?
There was a problem hiding this comment.
Great. In the meantime, I will test the update script.
|
I get an error when running the update script: still using graphdb 8.5 |
|
Ok, I know what happened: I loaded the data with this branch and applied the conversion. Instead, the data has to be loaded on develop and converted on this branch. Does this mean that the script breaks if the data is already converted? |
Yes. The solution to that is #1336. |
|
Did you manage to preserve the prefixes? They used to be renamed to ns0 or something. |
Merged
|
@benjamingeer Can this branched be updated, @SepidehAlassi as just merged a PR? |
|
@benjamingeer The upgrade script works! |
Cool. Horrible test almost done. |
|
@tobiasschweizer OK, I added a test to |
|
@tobiasschweizer Is this OK to merge? |
I want to get the feedback from @kilchenmann and @flavens. I think this should be possible today. |
|
@benjamingeer @tobiasschweizer You can merge this PR today. For you to know: We actually work only with the Knora official releases (now 7.0.0). But it is really good to know when there are breaking changes in the next release, this way, it is easier for us to fix bugs and adapt. |
|
@flavens Thank you! |
|
@tobiasschweizer In that case could you please approve this? |
tobiasschweizer
approved these changes
Jun 6, 2019
|
@tobiasschweizer Thank you! |
|
This pull request has been mentioned on Discuss DaSCH. There might be relevant details there: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR aims to make it possible to cite Knora values, as per this comment. See the discussion in Fine-grained citations.
knora-base:valueHasUUIDtoknora-base:Value. This is stored only in the current version of each value.KnoraIdUtilintoStringFormatter..ttlfiles.knora-base:valueHasUUIDto existing repositories.Existing repositories must be updated; see
upgrade/1322-cite-values/README.md.Resolves #941.