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

refactor(triplestore): remove graphDB support #2037

Merged
merged 18 commits into from Apr 13, 2022
Merged

Conversation

mpro7
Copy link
Collaborator

@mpro7 mpro7 commented Apr 5, 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-545

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

What I'm not sure to keep or delete are some parts od docs related to Inference and Consistency checking (in some places I left deprecation info) as well as updateLuceneIndex and SearchIndexUpdateRequest related code. Apart of that I removed all what was mentioned in the task, including twirl templates refactoring.

@mpro7 mpro7 self-assigned this Apr 5, 2022
@mpro7 mpro7 changed the title remove graphDB related scripts + clean docs refactor(triplestore): remove graphDB support Apr 7, 2022
@mpro7 mpro7 marked this pull request as ready for review Apr 7, 2022
Copy link
Collaborator

@BalduinLandolt BalduinLandolt left a comment

Nice! Very satisfying to review indeed! :D

Some small remarks are in the comments.

Apart from that I have two general remarks:

  • all the twirl templates have this triplestore parameter, which previously was used to discern which DB was in use. Now this parameter is never read. So IMO it could be removed from the templates; and with it, you'd have to remove it from all the places in Scala code, where the template is called with this parameter.
  • More broadly wondering, if we only support one triplestore, do we really need all this overhead for supporting multiple triplestores, like having the triplestore name in the config, etc.?

docs/04-publishing-deployment/updates.md Show resolved Hide resolved
@* Ensure that inference is not used in this query. *@
@if(triplestore.startsWith("graphdb")) {
FROM <http://www.ontotext.com/explicit>
}

Copy link
Collaborator

@BalduinLandolt BalduinLandolt Apr 7, 2022

Choose a reason for hiding this comment

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

in all those templates, the triplestore parameter should not be used anymore, and therefor should not have to be passed to the function anymore either

Copy link
Collaborator Author

@mpro7 mpro7 Apr 8, 2022

Choose a reason for hiding this comment

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

I'm still not sure if I should do that in this task, because this would also trigger removal of other things related to triplestore-specific code and documentation, which might be another Pandora box ;)

Copy link
Collaborator

@subotic subotic Apr 8, 2022

Choose a reason for hiding this comment

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

I think that removing the triplestore parameter would't be complicated, just more work. One would usually provide the value directly from settings to the template, e.g.,

...
triplestore = settings.triplestoreType,
...

So basically this line would need to be removed at every call-sight of a template.

@mpro7
Copy link
Collaborator Author

@mpro7 mpro7 commented Apr 7, 2022

@BalduinLandolt thanks for the comments and review. You have right in both cases. I actually noticed it too, however I wanted to keep it relatively simple and do not dig too deep avoiding problems to get this reviewed before holidays 😄 This PR is already very big, so maybe this could be done in another task or in this one, let's see...
Regarding config and names, actually we don't know if there is no plan to support more triplestores. No matter of that, prefixes with the triplestore name are fine, even if there is only one at the time.

Copy link
Collaborator

@irinaschubert irinaschubert left a comment

LGTM, satisfying indeed :)
Just one thing: What about the code smell, can you resolve it?

subotic
subotic approved these changes Apr 8, 2022
Copy link
Collaborator

@subotic subotic left a comment

LGTM, thank!

Just the one comment about the triplestore type. Either in this PR or the next, whatever you prefer.

@* Ensure that inference is not used in this query. *@
@if(triplestore.startsWith("graphdb")) {
FROM <http://www.ontotext.com/explicit>
}

Copy link
Collaborator

@subotic subotic Apr 8, 2022

Choose a reason for hiding this comment

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

I think that removing the triplestore parameter would't be complicated, just more work. One would usually provide the value directly from settings to the template, e.g.,

...
triplestore = settings.triplestoreType,
...

So basically this line would need to be removed at every call-sight of a template.

@mpro7 mpro7 force-pushed the DEV-545-remove-graph-db-support branch from a9e1155 to 39a09dc Compare Apr 13, 2022
@sonarcloud
Copy link

@sonarcloud sonarcloud bot commented Apr 13, 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 merged commit bf17bca into main Apr 13, 2022
11 checks passed
@mpro7 mpro7 deleted the DEV-545-remove-graph-db-support branch Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants