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

Distinguish Schema and Boolean case for AdditionalProperties in Schema #277

Merged
merged 1 commit into from
Sep 29, 2018

Conversation

jmini
Copy link
Contributor

@jmini jmini commented Sep 25, 2018

See Issue #257.

I think the TCK should test getter/setter/builder for additionalProperties in ModelConstructionTest.schemaTest(). I was surprised to see nothing there for the moment.
=> I will add a commit to this PR.

@jmini
Copy link
Contributor Author

jmini commented Sep 26, 2018

TCK test added with 0bf0c2d

@jmini jmini force-pushed the issue257_getAdditionalProperties branch from 0bf0c2d to c93cb49 Compare September 27, 2018 04:56
@jmini jmini changed the title Distinguish Schema and Boolean case for Schema.getAdditionalProperties() Distinguish Schema and Boolean case for AdditionalProperties in Schema Sep 27, 2018
assertEquals(s.getAdditionalPropertiesBoolean(), null, "AdditionalProperties (Boolean type) is expected to be null");
checkSameObject(s, s.additionalPropertiesBoolean(Boolean.TRUE));
assertEquals(s.getAdditionalPropertiesBoolean(), Boolean.TRUE, "AdditionalProperties (Boolean type) is expected to be true");
assertEquals(s.getAdditionalPropertiesSchema(), null, "AdditionalProperties (Schema type) is expected to be null");
Copy link
Member

Choose a reason for hiding this comment

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

I am trying to understand why the schema property would be null here, and not ap. Is it because we set the boolean property, and therefore would have automatically set the schema property to null? If so we should change the javadoc of the methods to specify this behaviour, and also change the default interface impl to set the other variant to null.

Signed-off-by: Jeremie Bresson <dev@jmini.fr>
@jmini jmini force-pushed the issue257_getAdditionalProperties branch from c93cb49 to 7aba41e Compare September 28, 2018 16:00
@jmini
Copy link
Contributor Author

jmini commented Sep 28, 2018

This OpenAPI spec snippet shows the possible values for additionalProperties:

  schemas:
    Test1:
      additionalProperties: false
    Test2:
      additionalProperties: true
    Test3:
      additionalProperties:
        type: object
        description: a person object
        properties:
          firstName:
            type: string
          lastName:
            type: string

(+ the null case, of course)

The additionalProperties is either a Boolean or a Schema but can not be both on the same time.

Because in Java we do not have union type, we decided to follow KaiZen approach: having getter, setters and builder as if there was 2 members additionalPropertiesBoolean and additionalPropertiesSchema.

In comparison swagger model just use Object and the consumer needs to know that can be made with the value (call instanceof Schema or instanceof Boolean to see what the additionalProperties member really is).

In my opinion the implementation needs to decide how it wants to handle this case in the setter and getter for additionalPropertiesBoolean, additionalPropertiesSchema.

1/ They can decide to store it in an Object member, and then do an instanceof + cast in the getter.

2/ They can decide to have 2 members, but then they need to null the value in the setter when the other value is set.

3/ Or they use their own storage layer (I did not verify it, but my understanding is that Kaizen does store everything in the JSON data structure).

The default implementation method only does a “delegation to the setter”. Doing anything else in those case might break this rule. The implementation needs to do it right anyway, it is not the responsibility of the builder.

Does this make sense?


Maybe I need to write the TCK test in an other way.


I have tried to update Javadoc:

  • I have added a precision in the getter.
  • In the setter and builder there is already a note about the method being “mutually exclusive” with the other variant. I have added a reference to the other method.

Copy link
Member

@arthurdm arthurdm left a comment

Choose a reason for hiding this comment

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

thanks @jmini - with your latest additions to the javadoc I think it's clearer now for users.

@arthurdm arthurdm merged commit 478c8a8 into eclipse:master Sep 29, 2018
Azquelt pushed a commit to Azquelt/microprofile-open-api that referenced this pull request Mar 17, 2022
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.

None yet

2 participants