Complex restrictions; enum change; cardinality + required improvements

[cweedall]

I started this change intending to remove the "items" and "type: array" structure. Basically, if there is restriction of exactly one for a property, then it shouldn't be an array. Instead, it should remove the "items" structure and have the type directly under the property name.

It got a little deeper than expected and I ended up looking at complex restrictions. Unfortunately, the code was set up to ignore complexity at/beyond certain levels (such as "owl:EquivalentTo"). After struggling awhile, I was able to get complex restrictions working with the RestrictionVisitor. This led me to fixing or updating several other things that were impacted.

Towards the end of the complex restrictions changes, I realized that enums could be handled in a better way (and maybe there is even a better way still?).

Finally, I added additional logic checking to handle cardinality generation in a way that I believe fits the intended API specification better. Some of this was mentioned above. For example, if restrictions indicate something is "exactly 1", then do not include it within an "items" array.

Additionally, if the maximum item value is 1 (and no minimum item value is set), then it is optional OR 1 - that means also don't include it under an "items" array.

If something is exactly one or has a minimum of 1 (or larger) requirement, then it should be treated as required.

Although the nullable value in OpenAPI is more meant to be a method to allow a value OR a null value, I also used the cardinality checks to assign a nullable boolean and treat it similar to an "is optional" value. I made some assumptions here, but this key/value pair is probably the most likely one to need manual setting after the OpenAPI spec is created.

Because you can set required properties for a class, I made some changes to the minItems/maxItems . These values are removed from the property of the specification already shows, for example, that a property has minItems: 1 and maxItems: 1. These key/value pairs are retained in other scenarios, such as when maxItems > 1.

I am not sure that the tests cover 100% of the updates... But I did update tests which broke because of some of these structural changes. From using several different ontologies, everything appears to generates correctly for me and without errors. However, I hope to revisit the tests soon and make sure more/all use cases are tested.

[dgarijo]

@cweedall thanks for your contributions.

removing the type array may be tricky, as it may break backwards compatibility. And you kind of push the problem to the client, no? Now they should either read the ontology to know what to expect, or know that some properties will return only one item, but others an array.

What if we always return an array, but if it's exactly one then we know that the size of the array must be 1?

[cweedall]

@dgarijo it would, as you mentioned, function differently than before. I am not sure I completely follow your comment about backwards compatibility. Can you explain this need more?

I found it a bit odd that everything was an array. I do not think I have seen any API specs where that is the case (I am ignoring the ontology and focusing on the API spec specifically, with this comment). I thought OBA may have been set to do it due to being easier to implement. From your comment, it appears that is not the case, at least for some users.

Regarding the other comment, can you explain the use case(s) where a user/client would need to be reading the ontology and the API spec and understanding this situation? I am trying to better understand this situation.

Finally, the request/suggestion to do it as an array of one item - I think that is what was happening before, if I recall correctly. I believe it could be re-set up in the current changes. However, I wonder if we should set up another configuration flag/parameter (similar to follow_references and the newer default_descriptions and default_properties) which can toggle this behavior versus the array way?

I am approaching this from the perspective of developing the ontology, using it to drive the API creation, and have customers who may not (and probably don't) have background with ontology but are still consuming/utilizing the API. Traditionally, APIs only use arrays when the key/property is an array (even if there is only 1 or 0 values for a specific request/response). In particular, I found the the API documentation is difficult to read when everything is an array, even though you know there's always just one item and it's required (and possibly nullable).

[dgarijo]

Hi Christopher, I mean that if I were to recreate the same APIs I did with existing ontologies prior to this change, my result would break my previous API implementations. I remember we discussed the decision on having everything as arrays back then. As someone consuming the API, it's is easier if the objects returned by it are consistent. Otherwise you kind of need to know which property you are dealing with, or add mechanisms in the client to differentiate whether something is a list or a single value. So something that may be a little more elegant for API designers may result in something a little bit more difficult to consume by people using the API . That said, I am happy to have this configurable with a flag and let the API designer decide what is it they want as you suggested. That would be the best compromise. Thanks again for your hard work! El mié., 1 may. 2024 3:52 p. m., Christopher Weedall < ***@***.***> escribió:

…

@dgarijo <https://github.com/dgarijo> it would, as you mentioned, function differently than before. I am not sure I completely follow your comment about backwards compatibility. Can you explain this need more? I found it a bit odd that everything was an array. I do not think I have seen any API specs where that is the case (I am ignoring the ontology and focusing on the API spec specifically, with this comment). I thought OBA may have been set to do it due to being easier to implement. From your comment, it appears that is not the case, at least for some users. Regarding the other comment, can you explain the use case(s) where a user/client would need to be reading the ontology and the API spec and understanding this situation? I am trying to better understand this situation. Finally, the request/suggestion to do it as an array of one item - I think that is what was happening before, if I recall correctly. I believe it could be re-set up in the current changes. However, I wonder if we should set up another configuration flag/parameter (similar to follow_references and the newer default_descriptions and default_properties) which can toggle this behavior versus the array way? I am approaching this from the perspective of developing the ontology, using it to drive the API creation, and have customers who may have background with ontology consuming/utilizing the API. Traditionally, APIs only use arrays when the key/property is an array (even if there is only 1 or 0 values for a specific request/response). In particular, I found the the API documentation is difficult to read when everything is an array, even though you know there's always just one item and it's required (and possibly nullable). — Reply to this email directly, view it on GitHub <#177 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALTIGSO5UFDJKFDPUVOP33ZADXRLAVCNFSM6AAAAABG64GTNCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBYGQ4TMMRQGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[cweedall]

Sounds good. Thank you for explaining and giving more background on that situation.

This PR will be on-hold temporarily with the intention of adding that flag and having it drive the property's generation in these cases. I expect it may take ~1 week.

I was working on improving the with configuration file flags are handled. Mostly so that adding new ones would be easier to implement and check. I will create a PR for that first and then merge that one into this branch before working on the things we discussed here.

I will re-comment here after changes are committed to this branch for review.

[cweedall]

@dgarijo Thank you for reviewing the configuration flags PR which I have merged into this branch.

I used the flags to differentiate between always generating arrays vs. not, depending on cardinality, per our discussion above / last week. Also closely tied in with this, I had added the required list of properties and realized that technically it would be adding a breaking change because it will add this list and not be exactly as generated before. Therefore, I added another configuration flag to enable/disable including this required property list.

Everything seems to be working as expected for me, but additional testing would be appreciated!

[cweedall]

I noticed that cardinality restrictions at the class level generate minItems and maxItems underneath the items: array. However, for some other properties (such as functional properties), these key/values appear directly under the property. For example, from example.owl in this repo:

    Person:
      example:
        value:
          id: some_id
      properties:
        identifier:
          items:
            type: string
          maxItems: 1
          nullable: true
          type: array
        address:
          items:
            maxItems: 2
            type: string
          nullable: true
          type: array
      type: object

I believe the functional property structure is the correct one. I will look into updating this for the other type(s) and then update the PR to be reviewed.

[cweedall]

Ok, sorry for the delay. After I created a workaround for the min/max items being on the property (rather than the item array), some unit tests were affected.

As I dug into these issues, I found there were some issues with MapperObjectProperty's getObjectPropertiesByRef() method. Not sure if this was introduced by me or not, but I was able to fix up the issue.

During these things, I ran into an issue where setting the "functional" flag was only set correctly in one place and hardcoded as false in some places within MapperSchema. It is not clear to me why they were hardcoded, but object and data properties can both be functional. So, it made sense to pass that flag every time.

I came upon an easier method to determine functionality. (Why re-invent the wheel? Plus it helps clean up the code a little bit). Using EntitySearcher.isFunctional(), you simply pass in the data property or object property expression plus the ontology/ies and it determines functionality for you.

[dgarijo]

Impressive work, thanks!