Clarify behaviour of methods that return list / map

[arthurdm]

A few methods return a list or map of children and are meant to be read-only. The issue is that we don't specify that in the javadocs, so it's up to interpretation of the vendors - we should clarify these scenarios to avoid inconsistent behaviour.

[arthurdm]

We agreed in today's hangout that these maps and lists should be immutable. This issue will drive the doc updates - and TCK additions.

[jmini]

See also discussion of this question in #235 for getOperations() in PathItem (called readOperationsMap() with 1.0.x). This is a convenience method that returns as one map the 8 Operations for the HTTP verbs (GET, POST, PUT…).

---

But there is even an other cases to consider: the maps that is returned by: OpenAPI.getPaths() or by Components.getSchemas()

If those maps are also immutable, then I think that you will need to have operation for mutation. Because something like this will no longer be possible:

components.getSchemas().put("Car", carSchema);

You need to do:

components.addSchema("Car", carSchema);

Then some methods to remove elements of each maps are missing.

[arthurdm]

good point @jmini - I agree that as part of this work we should ensure that each map has an equivalent add and remove route to update the collection.

[jmini]

This issue requires #248, so I think that it is a 2.0 feature.

@arthurdm : can you please update the label?

[arthurdm]

Good point. There is some work that we can do in 1.1 (insert javadoc clarification of immutable maps and add mutating operations where needed) for the existing maps, and in 2.0 when we convert some Map extensions into Map fields we should apply the same concept.

So it's both 1.1 and 2.0. Makes sense?

[jmini]

I think we have the same vision, but to sort out the issues. Le me rephrase what I mean.

---

insert javadoc clarification of immutable maps and add mutating operations where needed

This is part of #248.

---

This issue is about making everything immutable.

But as long as you have still Xxxxs extends Map<K,V> you can not control that getXxxxs() in the owner is immutable. Example: openAPI.getPaths()

When #248 is solved (the 2.0 version of the solution) we will have owner.getXxxxs().getYyyys() w hich returns the immutable Map. Example: openAPI.getPaths().getPathItems() and there will also be the mutating operations on path.

[arthurdm]

Yup, +1. In 1.1 we won't be able to block all the mutations from happening (since it extends map), but we can discourage and deprecate it via javadoc as a foreshadow of the changes coming in 2.0.

[jmini]

For lists, with 1.1 we should add a remove operation for each list.

---

Example getTags() method Operation in the operation interface.
(see methods for tags in the Operation interface).

Current:

• Operation addTag(String) already exists as builder method.

Modification with 1.1:

• void removeTag(String) will be added.
• getTags() can return an immutable list (this is an implementation decision, an implementation having already published a version for 1.0 will not change its behavior if it wants to stay backward compatible).

Modification with 2.0:

• getTags() must return an immutable list.

---

I found 15 getters methods that returns a list and that needs to be modified:

OpenAPI :

List<Server> getServers()
List<SecurityRequirement> getSecurity()
List<Tag> getTags()

Operation :

List<String> getTags()
List<Parameter> getParameters()
List<SecurityRequirement> getSecurity()
List<Server> getServers()

PathItem :

List<Server> getServers()
List<Parameter> getParameters()

Schema :

List<Object> getEnumeration()
List<String> getRequired()
List<Schema> getAllOf()
List<Schema> getAnyOf()
List<Schema> getOneOf()

ServerVariable :

List<String> getEnumeration()

[jmini]

I have opened PR #273 to add the removeXxxx(Yyyy) methods.

[jmini]

To continue to work on this on the 2.0 branch, the next step is to ensure that the getXxxx() returning lists are immutable at TCK level.

Should I write a test that ensures that getXxxx().add(xxxx) throw a runtime exception?

If the implementations uses Collections.unmodifiableList(list), then an java.lang.UnsupportedOperationException is thrown. But this might already be implementation details.

Any advice?

[arthurdm]

hey @jmini - the core idea is that the Map returned does not affect the internal map of the model.
Implementations may wish to return a clone, or an unmodifiableList - I would say that's an implementation detail.

With that in mind, the main check from the TCK would be that getXxxx().add(xxxx) does not affect the original Map, so essentially: getXxxx().add(xxxx).size() == getXxxx().size() + 1

[jmini]

I have opened PR #279 to add the removeXxxx(String) methods.

[jmini]

Related to this clarification topic, I have opened #284 to discuss the setter case.

[jmini]

For 2.0 I have implemented the check discussed in this comment #240 (comment) as PR #288

[jmini]

This issue can now be closed:

For 1.1 methods were added to manipulate lists and maps:

• lists: Add remove methods for lists #273
• maps: Add remove methods for maps #279

For 2.0 javadoc and tck were updated to indicates that the maps and lists getters returns a (potentially immutable) copy: #288

In my opinion nothing more to do here. @arthurdm can you close this?
Thank you a lot.

[arthurdm]

yup, thanks @jmini!