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

fix!: fix valueHasUri bad values and missing types (DEV-1036) #2079

Merged
merged 8 commits into from Jun 21, 2022

Conversation

mpro7
Copy link
Collaborator

@mpro7 mpro7 commented Jun 17, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: DEV-1036

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@mpro7 mpro7 changed the title add plugin, test and test data fix: fix valueHasUri bad values and missing types Jun 17, 2022
@mpro7 mpro7 changed the title fix: fix valueHasUri bad values and missing types fix: fix valueHasUri bad values and missing types (DEV-1036) Jun 17, 2022
@mpro7 mpro7 marked this pull request as ready for review June 17, 2022 12:16
@mpro7
Copy link
Collaborator Author

mpro7 commented Jun 17, 2022

@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 test-repository-upgrade is taking new plugin into the consideration, but on the LS maybe it will need to be started manually?

.gitignore Show resolved Hide resolved
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/> .
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. 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).

  2. Also, did you check if the existing test data needs to be changed?

Copy link
Collaborator Author

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
val newObjectValue = nodeFactory.makeDatatypeLiteral(literal.value, OntologyConstants.Xsd.Uri)

Copy link
Collaborator Author

@mpro7 mpro7 Jun 20, 2022

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.

@subotic
Copy link
Collaborator

subotic commented Jun 17, 2022

@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 test-repository-upgrade is taking new plugin into the consideration, but on the LS maybe it will need to be started manually?

@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".

Copy link

@irinaschubert irinaschubert left a 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).

@mpro7 mpro7 self-assigned this Jun 20, 2022
@mpro7
Copy link
Collaborator Author

mpro7 commented Jun 20, 2022

@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.

@mpro7 mpro7 requested a review from subotic June 20, 2022 06:01
Copy link
Collaborator

@BalduinLandolt BalduinLandolt left a comment

Choose a reason for hiding this comment

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

LGTM

@sonarcloud
Copy link

sonarcloud bot commented Jun 21, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@mpro7 mpro7 changed the title fix: fix valueHasUri bad values and missing types (DEV-1036) fix!: fix valueHasUri bad values and missing types (DEV-1036) Jun 21, 2022
@mpro7 mpro7 merged commit de1e5a4 into main Jun 21, 2022
@mpro7 mpro7 deleted the DEV-1036-dsp-api-unfindable-resource branch June 21, 2022 13:39
@daschbot daschbot mentioned this pull request Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants