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
Conversation
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.
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.?
webapi/src/main/scala/org/knora/webapi/responders/v2/SearchResponderV2.scala
Outdated
Show resolved
Hide resolved
webapi/src/main/scala/org/knora/webapi/store/triplestore/http/HttpTriplestoreConnector.scala
Outdated
Show resolved
Hide resolved
@* Ensure that inference is not used in this query. *@ | ||
@if(triplestore.startsWith("graphdb")) { | ||
FROM <http://www.ontotext.com/explicit> | ||
} | ||
|
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.
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
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'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 ;)
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 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.
webapi/src/main/twirl/org/knora/webapi/messages/twirl/queries/sparql/v2/getMapping.scala.txt
Outdated
Show resolved
Hide resolved
...knora/webapi/messages/twirl/queries/sparql/v2/getResourcesByClassInProjectPrequery.scala.txt
Outdated
Show resolved
Hide resolved
@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... |
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, satisfying indeed :)
Just one thing: What about the code smell, can you resolve it?
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, 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> | ||
} | ||
|
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 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.
a9e1155
to
39a09dc
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
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-545
What is the new behavior?
Does this PR introduce a breaking change?
Other information
What I'm not sure to keep or delete are some parts od docs related to
Inference
andConsistency checking
(in some places I left deprecation info) as well asupdateLuceneIndex
andSearchIndexUpdateRequest
related code. Apart of that I removed all what was mentioned in the task, including twirl templates refactoring.