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
fix!: fix valueHasUri bad values and missing types (DEV-1036) #2079
Conversation
@subotic this isn't breaking change, because it's just plugin to fix bad data. There will be no major version increase, therefore ontology version wasn't updated too. I checked locally |
webapi/src/main/scala/org/knora/webapi/store/triplestore/upgrade/RepositoryUpdatePlan.scala
Outdated
Show resolved
Hide resolved
test_data/upgrade/pr2079.trig
Outdated
knora-base:standoffTagHasStartIndex 2 ; | ||
knora-base:standoffTagHasStartParent <http://rdfh.ch/0103/5LE8P57nROClWUxEPJhiug/values/fEbt5NzaSe6GnCqKoF4Nhg/standoff/1> ; | ||
knora-base:standoffTagHasUUID "dd2e785b-041e-4bcb-a77d-570efe8a068c" ; | ||
knora-base:valueHasUri <http://www.maison-george-sand.fr/> . |
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.
-
did you check, if the standoff handling actually requires or can handle typed values? The value at line 29 is a correct IRI in pointy brackets. You could check it by for example changing existing test data and then running the tests (if this case is covered).
-
Also, did you check if the existing test data needs to be changed?
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 don't know how/where to check but in LS data there are only 2 instances of data written like in the code snippet above. Any other valueHasUri
used in standoff looks like that:
knora-base:valueHasUri "https://lumieres.unil.ch/fiches/bio/2666/"^^xsd:anyURI .
In our test data I found only one example which is typed string:
https://github.com/dasch-swiss/dsp-api/blob/main/test_data/all_data/anything-data.ttl#L1244
case literal: DatatypeLiteral => | ||
if (literal.datatype != OntologyConstants.Xsd.Uri) { | ||
statementsToRemove += statement | ||
|
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.
val newObjectValue = nodeFactory.makeDatatypeLiteral(literal.value, OntologyConstants.Xsd.Uri) |
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.
Thanks for suggestion, but it needs to take parameter because in another case it takes differently named value to create DatatypeLiteral
.
.../src/main/scala/org/knora/webapi/store/triplestore/upgrade/plugins/UpgradePluginPR2079.scala
Outdated
Show resolved
Hide resolved
.../src/main/scala/org/knora/webapi/store/triplestore/upgrade/plugins/UpgradePluginPR2079.scala
Outdated
Show resolved
Hide resolved
@mpro7 AFAIK there is no way of running the upgrade scripts manually. So the only way is to bump the ontology version number, which then necessitates to bump the mayor number of DSP-API. I think that at some point I said, that each time an upgrade script needs to be run it is seen as a breaking change, because without the upgrade script, the system is broken, which is exactly the state in which we are now. Also, we define what breaking change means. It serves a purpose. In this case, it should scream at the person who is deploying: "look at the logs to see if the upgrade went through without problems" and "this time deployment will take longer than usual". |
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.
LGTM, just some cosmetic suggestions. I would also recommend to double check if it works with standoff (but Ivan already mentioned that).
.../test/scala/org/knora/webapi/store/triplestore/upgrade/plugins/UpgradePluginPR2079Spec.scala
Outdated
Show resolved
Hide resolved
.../test/scala/org/knora/webapi/store/triplestore/upgrade/plugins/UpgradePluginPR2079Spec.scala
Outdated
Show resolved
Hide resolved
@subotic I thought it is possible to run scripts on servers so this could be run like that - manually. but if you say so I changed it to be the breaking change and updated version numbers. |
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.
LGTM
.../src/main/scala/org/knora/webapi/store/triplestore/upgrade/plugins/UpgradePluginPR2079.scala
Show resolved
Hide resolved
Kudos, SonarCloud Quality Gate passed!
|
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: DEV-1036
What is the new behavior?
Does this PR introduce a breaking change?
Other information