KnowledgeCaptureAndDiscovery / OBA

Ontology based APIs
Apache License 2.0
33 stars 11 forks source link

RestrictionVisitor rewrite requires careful review #181

Open cweedall opened 4 months ago

cweedall commented 4 months ago

I created a draft PR #180. There was a lot to cover and I included those discussion points in the PR.

The reason for this issue is because the code changes are likely not to be easily comparable. For example, RestrictionVisitor ended up be largely a fully rewrite (although, most of the code still remains, it was condensed and helper methods were used - therefore it looks very different overall).

After comparing three ontologies before the rewrite and using the rewrite .jar, everything seems to be generated as I expected. Mostly the same overall, including some bug fixes.

For some things (such as hasDegree for ProfessorInArtificialIntelligence referring to a subset [i.e. MS and PhD] of the Degree enum from example.owl), I made some assumptions and it renders the OpenAPI spec slightly differently. I may have made incorrect assumptions. Or, there may be a need/interest for both possibilities. I also would like help verifying if I made any errors.

I can mark the PR ready for review after any concerns are addressed. Thank you in advance!

dgarijo commented 3 months ago

Thanks, I will have a look as soon as I can!

cweedall commented 2 months ago

Alright... it took longer than expected. A combination of life and trying to handle all the different ontology scenarios that I ran into. I utilized many of the changes from draft PR #180 into: draft PR #182.

This ended up being a major effort. I did my best to test it. But there may be things I missed.

The main thing is that complex properties are supported. So, if you see some values with a union or intersection, the property's items structure should be look similar to the following, for an object property:

propertyName:
  ...
  items:
    anyOf: #some values from
      anyOf/allOf: #union of / intersection of
        - $ref: "class reference"