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

Clarify behaviour of setter/builder methods with list / map parameter #284

Closed
jmini opened this issue Oct 9, 2018 · 4 comments
Closed

Comments

@jmini
Copy link
Contributor

jmini commented Oct 9, 2018

When a list or a map instance is passed to as setter method (or a builder) the implementation should create a copy of the items.

Examples:

  1. setting an immutable list map
List<Parameter> parameters = Collections.singletonList(pathParam);
operation.setParameters(parameters);
operation.addParameter(queryParam); // should work.

In my opinion this should work.

  1. putting an element after having passed the list map instance.
Map<String, Callback> callbacks = new LinkedHashMap<>();
callbacks.put("myCallback", callback);
operation.setCallbacks(callbacks);

callbacks.put("myCallback2", callback2);

operation.getCallbacks(); // should only contains 'myCallback'

In my opinion the second addition in the map should not modify the map in the OpenAPI instance.

For both cases, this might be a decision that is taken at implementation level (I am open for discussion).


My propositions:

For 1.1:

Relax the TCK in oder to allow implementation to do some copy of map/list internally. Right now some code position in ModelConstructionTest are assuming to get the same instance.
The check should check for equality instead, because the list/map content must be the same but the list/map instance can be an other one.

NB: I have prepared the change, but I made it on top of #283, so lets go with this first.

For 2.0:

Depending on the consensus, the TCK should enforce the implementation to do a copy of the map in the list/map setters.

I think it would be better so, but this is subject to debate.

I can accept this to be only a constraint that I add for my own implementations.

@arthurdm
Copy link
Member

I agree @jmini. Implementations should copy the contents of the list/map, not use the actual instance.

jmini added a commit to jmini/microprofile-open-api that referenced this issue Oct 13, 2018
See eclipse#284

Signed-off-by: Jeremie Bresson <dev@jmini.fr>
@jmini
Copy link
Contributor Author

jmini commented Oct 13, 2018

PR for 1.1: #286

@jmini
Copy link
Contributor Author

jmini commented Oct 24, 2018

PR for 2.0: #293

@jmini
Copy link
Contributor Author

jmini commented Nov 5, 2018

Both PRs are merged. Closing this.

@jmini jmini closed this as completed Nov 5, 2018
Azquelt pushed a commit to Azquelt/microprofile-open-api that referenced this issue 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

No branches or pull requests

2 participants