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

Implement smart IRIs #662

Merged
merged 38 commits into from
Nov 20, 2017
Merged

Implement smart IRIs #662

merged 38 commits into from
Nov 20, 2017

Conversation

benjamingeer
Copy link

@benjamingeer benjamingeer commented Nov 7, 2017

  • Add a SmartIri class to simplify IRI inspection, validation, and conversion.
  • Rename methods in StringFormatter to clarify which ones inspect (is*), which ones validate (validate*), and which ones convert (to*).
  • Use SmartIri for the IRIs of ontologies and ontology entities in API v2 code.
  • Use SmartIri for all IRIs in KnarQL.
  • Don't use v2 ontology info classes in v1 code, because that would mean changing a lot of v1 code to use SmartIri. Instead, add v1 wrappers around v2 ontology info classes. In the wrappers, convert smart IRIs to string IRIs. Use only the wrappers in v1 code.
  • Refactor v2 ontology info classes so they can support both read and write operations using the wrapper pattern. The read wrappers can convert themselves from internal to external, and the wrapped classes can convert themselves from internal to external and vice versa.
  • Separate schema conversion from JSON-LD generation. When serving ontology info, first convert IRIs from internal to external as needed, generating a new ReadEntityDefinitionsV2 instance. Then convert that instance to JSON-LD.
  • For better performance, cache SmartIri instances representing Knora ontology entities..

Benjamin Geer added 5 commits November 7, 2017 15:37
@benjamingeer benjamingeer changed the title Ontology update infrastructure part 5 Implement smart IRis Nov 8, 2017
@benjamingeer benjamingeer changed the title Implement smart IRis Implement smart IRIs Nov 8, 2017
Benjamin Geer added 6 commits November 10, 2017 12:22
# Conflicts:
#	webapi/src/main/scala/org/knora/webapi/util/KnoraIdUtil.scala
#	webapi/src/main/scala/org/knora/webapi/util/StringFormatter.scala
#	webapi/src/test/scala/org/knora/webapi/responders/v2/OntologyResponderV2Spec.scala
#	webapi/src/test/scala/org/knora/webapi/util/StringFormatterSpec.scala
- Remove IRI caching, because performance is good anyway.
- Put back IRI caching, but only for Knora definition IRIs.
- I think OntologyV2R2RSpec test data is wrong (has DatatypeProperty where it should have ObjectProperty).
@benjamingeer
Copy link
Author

@tobiasschweizer SearchRouteV2R2RSpec still has errors, and I need your help to understand why:

org.knora.webapi.NotImplementedException: Value type http://www.knora.org/ontology/knora-base#DateValue not supported in FilterExpression
	at org.knora.webapi.responders.v2.SearchResponderV2$AbstractExtendedSearchTransformer.transformFilterExpression(SearchResponderV2.scala:832)

Looking at that part of the code in SearchResponderV2, it's looking for things like OntologyConstants.KnoraBase.Date, which doesn't seem like it should exist. There are comments like this in OntologyConstants:

val Date: IRI = KnoraBasePrefixExpansion + "Date" // TODO: find a better solution for v2 simple

Could you explain? I'm guessing that you did this because it would have been more work to convert the IRI knora-api:Date to the IRI knora-base:DateValue. But this is done automatically now by SmartIri. So should this code in SearchResponderV2 now look for knora-base:DateValue?

@tobiasschweizer
Copy link
Contributor

tobiasschweizer commented Nov 13, 2017

The reason is that in a KnarQL query the user would use simple Iris like a knora-api:Date that will be converted to a knora-base:DateValue. The main rationale is hiding the value object complexity from the user (to her it is simply a date, not a value object). However, we could surely discuss if omitting Value makes a difference at all.

In SearchResponderV2 there are many places where Iris have to converted from external to internal (when looking at type annotations) or from internal to external (when comparing an already converted Iri to the given type annotations). It would make the code much clearer if smart Iris handled that automatically. We can also look at that later today.

@tobiasschweizer
Copy link
Contributor

Looking at that part of the code in SearchResponderV2, it's looking for things like OntologyConstants.KnoraBase.Date, which doesn't seem like it should exist.

Yes, it was kind of a hack ;-)

@benjamingeer
Copy link
Author

benjamingeer commented Nov 13, 2017

Maybe my question wasn’t clear. I’m asking why the search responder refers to an entity called http://www.knora.org/ontology/knora-base#Date instead of http://www.knora.org/ontology/knora-api/simple/v2#Date. There is no Date in knora-base, only in knora-api/simple/v2. Why does the search responder refer to a nonexistent Date entity in knora-base? Why doesn’t it use the Date entity that does exist in knora-api/simple/v2?

SmartIri can convert http://www.knora.org/ontology/knora-base#DateValue to http://www.knora.org/ontology/knora-api/simple/v2#Date. Perhaps that is what you need?

@tobiasschweizer
Copy link
Contributor

Simply because the external Iri (type annotation) is converted to an internal one before looking at it, using a regex. And the regex just replaces a part of the Iri, but does not handle the missing Value:

https://github.com/dhlab-basel/Knora/blob/9236b0fca594dc035e39b31ae56dbb292cd2058f/webapi/src/main/scala/org/knora/webapi/responders/v2/SearchResponderV2.scala#L602

However, this is not really consistent behavior since there are also non Knora types like xsd:string that cannot be converted.

So I would be happy to change this.

@benjamingeer
Copy link
Author

So I would be happy to change this.

OK, could you please do it on this branch? Then I think SearchRouteV2R2RSpec should pass.

I already changed the KnarQL parser and type inspector so they use SmartIri. Once you have a SmartIri, you can just call toOntologySchema on it to convert it from external to internal and vice versa.

@tobiasschweizer
Copy link
Contributor

OK, could you please do it on this branch? Then I think SearchRouteV2R2RSpec should pass.

I will look at it :-)

…SmartIri.

- Remove old API v2 plans doc, which has already been implemented and superseded.
- Add route for getting ontology metadata per project.
- Add R2R tests for getting ontology metadata.
- Rename "namedgraph" to "allentities" in ontology routes.
- Add KnoraContentV2 trait with toOntologySchema method.
@benjamingeer
Copy link
Author

@tobiasschweizer I renamed the /ontologies/namedgraphs route to /ontologies/allentities. The route that just returns ontology metadata is now called /ontologies/metadata.

@tobiasschweizer
Copy link
Contributor

@benjamingeer Thank you very much for the notification. I will make a new branch in SALSAH that will reflect this change and other changes on this branch that effect the routes or the messages.

Actually, we need to do coordinated merges: basically you need to open both pull requests and hit the merge buttons at the same time. I guess we need tentacles for this.

@benjamingeer
Copy link
Author

@tobiasschweizer I started added some API v design docs, including a discussion of SmartIri, in docs/rst/knora-api-server/design-documentation/api-v2.rst. See what you think.

@tobiasschweizer
Copy link
Contributor

Ok, I look at it now. Just making myself a cappuccino ...

Each API response is represented by a class that extends ``KnoraResponseV2``, which
has a method ``toJsonLDDocument`` that specifies the target schema. It is currently
up to each route to determine what the appropriate response schema should be. Some routes
will support only one respnse schema. Others will allow the client to choose, and there will
Copy link
Contributor

Choose a reason for hiding this comment

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

respnse -> response

contains the value's IRI. When a value is created, it is wrapped in a ``CreateValueV2``, which has
the resource IRI and the property IRI, but not the value IRI.

A ``Read*`` wrapper can be wrapped in anotehr ``Read*`` wrapper; for example, a ``ReadResourceV2``
Copy link
Contributor

Choose a reason for hiding this comment

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

anotehr -> another

@tobiasschweizer
Copy link
Contributor

I had a look at it and I think that it is very useful!

@tobiasschweizer
Copy link
Contributor

I am adding more KnarQL R2R tests now to make sure that the changes on this branch do not have unintended effects.

@tobiasschweizer
Copy link
Contributor

haha, found a bug in my code!!!!!!!!

@dasch-swiss dasch-swiss deleted a comment from tobiasschweizer Nov 20, 2017
@dasch-swiss dasch-swiss deleted a comment from tobiasschweizer Nov 20, 2017
@benjamingeer benjamingeer merged commit cc53ef8 into develop Nov 20, 2017
@benjamingeer benjamingeer deleted the wip/ontology-updates-5 branch November 20, 2017 16:10
@subotic subotic added this to the v1.1.0 milestone Feb 12, 2018
@benjamingeer benjamingeer mentioned this pull request Apr 23, 2018
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.

3 participants