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

Schema.getAdditionalProperties(): return something better as Object? #257

Closed
jmini opened this issue Aug 20, 2018 · 9 comments
Closed

Schema.getAdditionalProperties(): return something better as Object? #257

jmini opened this issue Aug 20, 2018 · 9 comments

Comments

@jmini
Copy link
Contributor

jmini commented Aug 20, 2018

Per spec: additionalProperties in a Schema object can be an Object or a Boolean.
See OAI/OpenAPI-Specification#668 (comment)

In the code, Object is returned:

This is not really user-friendly because the user needs to know that this Object can be a Schema or a Boolean and write code with instanceOf to reflect this.


Kaizen Model has 2 getters for this:

  • Boolean getAdditionalProperties()
  • Schema getAdditionalPropertiesSchema()

I do not like this either, but it is better in a sense that no instanceOf is needed.

kaizen-parser Schema.java


What is the typical java way of telling Schama or Boolean (a sort of Union Types)?
A new Interface AdditionalProperties with 2 getters returning an optional:

  • Optional<Schema> getAsSchema()
  • Optional<Boolean> getAsBoolean()
@arthurdm
Copy link
Member

Discussed in Sept 10 hangout. +1 to the way Kaizen's has done it.

Boolean getAdditionalProperties()
Schema getAdditionalPropertiesSchema()

@arthurdm
Copy link
Member

1.1 will deprecate getAdditionalProperties and introduce the 2 new methods, while 2.0 will remove getAdditionalProperties

@jmini
Copy link
Contributor Author

jmini commented Sep 11, 2018

Thank you for discussing this in the Hangout, and thank you for reporting here the decision.

Current (1.0):

Object getAdditionalProperties()

Is the idea to have:

Boolean getAdditionalPropertiesBoolean()
Schema getAdditionalPropertiesSchema()

Then I understand that getAdditionalProperties() needs to be removed with 2.0 (and can be deprecated with 1.1).

I prefer this solution, because if users are using Object o = s.getAdditionalProperties() it is not clear that the method now only returns Boolean.


But you have mentioned:

Boolean getAdditionalProperties()
Schema getAdditionalPropertiesSchema()

Then the only modification is that getAdditionalProperties() was returning an Object with 1.0 and will return a Boolean with 2.0.

@jmini
Copy link
Contributor Author

jmini commented Sep 13, 2018

Additional proposition:

We have also already 2 setters:

/**
* Sets the Schema which defines additional properties not defined by "properties" or "patternProperties".
* See the javadoc for {@link Schema#getAdditionalProperties()} for more details on this setting. Note
* that this version of the setter is mutually exclusive with the Boolean variants.
*
* @param additionalProperties a Schema which defines additional properties
*/
void setAdditionalProperties(Schema additionalProperties);
/**
* Sets the value of "additionalProperties" to either True or False. See the javadoc for
* {@link Schema#getAdditionalProperties()} for more details on this setting. Note that
* this version of the setter is mutually exclusive with the {@link Schema} variants.
*
* @param additionalProperties a Schema which defines additional properties
*/
void setAdditionalProperties(Boolean additionalProperties);

I think they should be renamed to:

Boolean setAdditionalPropertiesBoolean(Boolean)
Schema setAdditionalPropertiesSchema(Schema)

In order to be consistent with the getters.

@arthurdm
Copy link
Member

Good catch @jmini

We discussed in today's hangout and agreed with the additions of:

Boolean getAdditionalPropertiesBoolean()
Schema getAdditionalPropertiesSchema()
Boolean setAdditionalPropertiesBoolean(Boolean)
Schema setAdditionalPropertiesSchema(Schema)

And deprecate the existing ones in 1.1, removing then in 2.0

@jmini
Copy link
Contributor Author

jmini commented Sep 25, 2018

PR for this: #277. Feedback welcomed.

@jmini
Copy link
Contributor Author

jmini commented Sep 29, 2018

PR for 2.0: #281

@jmini
Copy link
Contributor Author

jmini commented Oct 22, 2018

Both PRs has been merged (for 1.1 and 2.0). Nothing more to do in my opinion, I am closing this issue. Thank you a lot.

@jmini jmini closed this as completed Oct 22, 2018
@jmini
Copy link
Contributor Author

jmini commented Oct 22, 2018

Both PRs has been merged (for 1.1 and 2.0). Nothing more to do in my opinion, I am closing this issue. Thank you a lot.

@arthurdm arthurdm added this to the MP OpenAPI 2.0 milestone Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants