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

Rewrite object/restriction visitor to handle all complex scenarios. Some fixes. Add new configuration options. #182

Open
wants to merge 71 commits into
base: master
Choose a base branch
from

Conversation

cweedall
Copy link
Contributor

@cweedall cweedall commented Jun 25, 2024

I bumped a major version (4.0.0) due to the combination of updates.

Bumped some package versions to the newest version.

Config options + documentation:
New option to generate JSON instead of YAML. (I thought about trying to support JSON as a config file... but it's more in-depth than I would like to go).
New kebab case flag (current/default is flatcase - all lowercase and no spaces; kebab case is industry standard for endpoint names).
New inheritance reference flag - if true, class will inherit via allOf reference to a super/parent class.

In general, removed references to class member variables and referenced them via a method. A mixture of both was in use before and it's simply easier to stay consistent, especially for a somewhat smaller codebase. Wherever I noticed a member variable reference in a class, I added this. for clarity. A somewhat stylistic choice, but I shifted to camelCase and away from snake_case (the use of camel case is more prevalent in my experience, and soe this was more of a general industry consistency decision.). A shift to using Sets instead of Lists because we shouldn't be using lists where duplicates of the same thing appear (owlapi generally does this... but for some reason they allow/use Lists for certain things like allOf, anyOf, etc. which I find a bit weird...).

I think I accidentally broke the description getter in the previous update. This is now fixed. Class and property descriptions look good again. I also added the DSTERMs namespace to get descriptions defined with it which were not previous gotten.

Restructured the property mappers. Except for schema object/data property schema creation, make everything static. New generic MapperProperty for common functionality of object/data properties. Extend that class by MapperObjectProperty and MapperDataProperty. Avoid confusion switch cases. Use individual static adder/setter methods to modify schema details (e.g. cardinality) and handle different data constructs (e.g. oneOf, allOf, anyOf, etc.).

Update MapperOperation to use links from the endpoint description to the associated model schema. Some media type updates. Some placeholder commented code for example generation. Currently, each endpoint now has a reference to the model schema that represents it. Technically, this is invalid, so an exception in the validator was added. It actually generates nicely in Swagger's UI and makes the process much easier, since we don't need to worry about determing example values for a property. I think(?) this is a bug somewhere and either the validator is wrong or the UI generation is wrong (and may get fixed in the future?).

The Mapper class was cleaned up a bit and directly calls the ObjectVisitor (previously RestrictionVisitor).

The MapperSchema class mostly duplicated work that Mapper was doing or that the ObjectVisitor/RestrictionVisitor should be doing. A majority was moved to ObjectVisitor because it made the most sense and was supported under the abilities of OWLObjectVisitor. This class was deleted after moving the functionality.

As noted above, RestrictionVisitor was renamed to ObjectVisitor. This was partly done because it was already extending OWLObjectVisitor and to keep the naming more consistent. As I was comparing with MapperSchema I noticed that it made more sense to move a lot of the looping and searching for domains/ranges into ObjectVisitor because they were tightly-coupled. The idea was to get each property and then do restrictions, where applicable. In some cases, getting the restrictions and getting the property were the same (and therefore, the process duplicated). This could occur, for example, if an ontology defines restrictions for a property and does not define any range for the property.

I noticed that inheritance of object/data properties did not seem to work correctly. If you have a super property, for example, with a domain and the subproperty has a range, the subproperty would not correctly be generated in the OpenAPI spec. The object properties required the reasoner to calculate this things. It was a bit weird, but was done by calculating direct subproperties of the owl:topObjectProperty, keeping track of any domains/ranges and then traversing deeper, if there are any deepar subproperties.

cweedall added 30 commits May 9, 2024 14:53
@dgarijo
Copy link
Contributor

dgarijo commented Jun 29, 2024

Thanks @cweedall,
this is becoming too big to review. Do you think it can maybe be broken down a bit?
I will wait until you mark it ready

@cweedall
Copy link
Contributor Author

cweedall commented Jul 1, 2024

I thought about treating it as a fork instead, because it is difficult to compare code-wise. There are some small things that I added along the way, such as the configuration flags, that could be removed - but these changes also are so small that it would not really impact the big-ness of the change.

I have not been able to think of a good way to split this up to have it reviewed in several steps. Part of the problem was that RestrictionVisitor/ObjectVisitor was intertwined a lot with MapperSchema, Mapper, MapperObjectProperty, and MapperDataProperty. For many seemingly small updates, it cascaded into changing 2-4 files.

@cweedall
Copy link
Contributor Author

cweedall commented Jul 3, 2024

@dgarijo I started looking into breaking this down into smaller pieces. My impression so far is that it would take a lot of work and probably take longer to separate than these changes took.

I'll mark it ready to review. But if you and the other repo owners decide it is not reviewable or prefer not to merge, I will understand. In that case, I will set it up as a fork and perhaps it could be revisited in the future.

@cweedall cweedall marked this pull request as ready for review July 3, 2024 15:24
@dgarijo
Copy link
Contributor

dgarijo commented Jul 3, 2024

That's ok, I will have a look soon

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.

2 participants