-
Notifications
You must be signed in to change notification settings - Fork 12
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
Restriction visitor rewrite supports most complex restrictions. Change most lists to sets. Some mapper updates to deal with those changes #180
base: master
Are you sure you want to change the base?
Conversation
Thanks, @cweedall impressive work! |
@dgarijo Thanks for reviewing. I know you mentioned putting off complex unions/intersections, but I ended up finding a way to do it after posting this. I think I will orphan this branch, but leave it available for some time. I have new changes that are heavily based on this, but also include wrapping the MapperSchema class into the Visitor class. There was a lot of stumbling, but I found ways to make the code more readable and minimize repeated steps/loops. Regarding the link, I had come across that previously. As I was comparing the coding work with ontologies, I came across instances that I was unsure about. There may be some others, but from what I can recall at the moment, examples included:
allOf:
- $ref: '#/components/schemas/SCHEMANAME'
- type: object I would like to better understand some of these details. And, if have made any mistakes it is definitely good to fix these! Some/all of these answer may be listed there or elsewhere in the literature(?) Unfortunately, I do not have a lot time to familiarize myself thoroughly with the literature and would greatly appreciate direction on where to look. After I have time to wrap up some of my changes, mentioned at the top, I will post a new draft PR. Hopefully I can incorporate any problems/confusions that I had in this branch, and the questions here, into the new one. Thank you again for your patience with helping and reviewing! |
This PR will be very difficult to review. Apologies in advance for this!
I was looking through other issues and noticed issue #93 Range with complex objects (using other schemas) makes OBA not recognize all classes. I noticed the range for
asociadaA
was aunionOf
two classes and it was being used at least once with exact cardinality. This got me started looking to see if I could further fix/improve complex restrictions.The closer I got to it working correctly, the more I realized I was reusing/condensing a lot of the existing
RestrictionVisitor
code. I ended up with 3 helper methods to store the properties/restrictions in the Set and Map variables. Several visit types (e.g.OWLNaryBooleanClassExpression
versusOWLObjectUnionOf
andOWLObjectIntersectionOf
) were virtually identical, except for the restriction values key. For all of these, I made helper methods and inferred the key based on the subinterface.Also added comments and fixes where it became easier to identify issues due to code cleanup. For this re-write, it also eventually became clear that enums are basically class restrictions (rather than properties) and can be handled here also. The caveat is that the property is an empty string (because it's not a property!). This is treated as a special case check in
MapperSchema
, which builds the enum schema upon encountering this restriction.There were some adjustments that needed to be made in
Mapper
andMapperSchema
due to theRestrictionVisitor
changes. Perhaps the biggest issue that I ran into when using the ontology from issue #93 Range with complex objects (using other schemas) makes OBA not recognize all classes, is that it didn't generate endpoints or models for classes based on the ontology's IRI. This may require some discussion.Throughout, I updated most of the lists to sets. I did this partly by comparing examples from owlapi TutorialSnippetsTestCase and partly by realizing this allowed us to only pass the
OWLClass
object around and determine which ontology it was part of. (There may be a better way overall, but I was trying to make incremental improvements, including readability and code usability/write-ability).Lastly, I encountered several things the appeared to be bugs. For example, data type
nonPositiveInteger
would map to theinteger
type in the spec - but if you havenonPositiveInteger
andnonNegativeInteger
then you'd have two (generic) integer entries in theitems
list; this completely misses the fact that it is basically saying "only zero is allowed"."oneOf" references to enum values should just use the individual's short form (in my opinion) like the enum classes and not use
format: uri
andtype: string
, in order to align with the enums. But, if this is necessary, I am OK with using a configuration flag to flip between the two methods.Unions and intersections appear to be working correctly now for me, but I was seeing bugs (i.e. missing OpenAPI spec details) in some cases, such as
StudyMaterial
from theexample.owl
file. Inheritance was also not working in some cases. For example,hasDegree
was missing onStudent
, despite it existing on its superclassPerson
.The only two issues that I am aware of from restrictions and schema models now:
testObjectUnionOf
inexample.owl
) createanyOf
ANDallOf
arrays underitems
- each having all of the range values conflated together. (To fix this will require a re-write ofRestrictionVisitor
and the mapper classes and be a big undertaking.)