Models: do we need the interfaces extending Map?

[jmini]

There are several model interfaces:
Paths, ServerVariables, Content, APIResponses, Scopes, Callback,
that extends, Map<String, T>

My points are:

1/ Those Interfaces are really strange, when working with it, you never can not use stuff like:

• Arrays.asList(..)
• Streams + Collectors

2/ To implement #240 we might need to remove those interface. How do you create an immutable version of Paths

3/ The KaiZen-Parser models do not use such a construct.

[EricWittmann]

Yep - I think the only time we should have an interface that extends Map is if the collection is extensible. I haven't checked all the interfaces provided above, but I know that the ServerVariables are not extensible. Basically, if the OpenAPI spec lists something as Map[string,X] then I think we should model it as a simple Map in the model layer.

[jmini]

@EricWittmann : thank you for the "extensible" pointer.

I divided my list into two categories:

1) Not extensible.

No extends Extensible in the current code:

org.eclipse.microprofile.openapi.models.media.Content
org.eclipse.microprofile.openapi.models.responses.APIResponses

For:

org.eclipse.microprofile.openapi.models.servers.ServerVariables

As discussed in #245 it is marked as extensible for the moment, but this is wrong according to the spec.

2) Extensible.

org.eclipse.microprofile.openapi.models.Paths
org.eclipse.microprofile.openapi.models.callbacks.Callback
org.eclipse.microprofile.openapi.models.security.Scopes

Extensible in the current code and marked as such in the OpenAPI-Spec.

I would like to understand how an extension on the Paths or Scopes looks like. A comparison with KaiZen is also requested.

[arthurdm]

+1 for replacing the non-extensible model interfaces with a map version in the appropriate encapsulating model.

For the extensible (as per OAS3 spec) model interfaces, I propose that we don't extend from Map, and instead just contain a map as an internal field - exposing the non-mutable collection, and appropriate add and remove methods.

[arthurdm]

For 1.1 we can add the map fields, add and remove methods and put a deprecating notice at the appropriate places.

I opened #251 to start tracking future 2.0 candidates - I think this one fits that.

[jmini]

org.eclipse.microprofile.openapi.models.responses.APIResponses are extensible, see #254

[jmini]

For 1.1 we can add the map fields, add and remove methods and put a deprecating notice at the appropriate places.

I am OK with this, can we sketch how the methods should look like?

Example with Paths:

Current situation:

• Paths is currently a extending Map<String, PathItem>.
• method to add a single element Paths addPathItem(String name, PathItem item)

Operation that can be added with 1.1:

• getter for the map: Map<String, PathItem> getPathItems() can returns itself with 1.1 as a transition. With 2.0 this method will return an immutable copy of the items.
• setter: void setPathItems(Map<String, Path> paths)
• has element check method: boolean hasPathItem(String name)
• get single element method: Path getPath(String name)
• remove single element method: void removePathItem(String name)

Not sure how we can make clear that the java.util.Map method should no longer be used with 1.1

With version 2.0 we will remove extends Map<String, PathItem>

Is this what you have in mind?

[arthurdm]

Yup, that's what I was thinking about. I wonder if setPathItems(Map<String, Path> paths) should return Paths instead of void?

For 1.1, perhaps we could add a Javadoc comment at the class level saying that use of methods inherited from Map are deprecated in favor of the methods declared in the Paths class itself.

[jmini]

I wonder if setPathItems(Map<String, Path> paths) should return Paths instead of void?

If it returns Paths then it should be called pathItems(Map<String, Path>) to be consistent with the other builder methods...

[jmini]

To continue the discussion, I have opened #266 where I have made the change for Paths (for the version 1.1)

[arthurdm]

Agreed in the Sept. 10 hangout with the agreed direction of deprecating the Map extension in 1.1 and removing the extension in 2.0

[jmini]

For the discussion in previous comment #248 (comment)

Was something said in the Hangout?

---

In PR #266, I have decided to use:

Paths addPathItems(Map<String, PathItem> items);

Instead of

void setPathItems(Map<String, PathItem> items);

Is this the way to go?

[arthurdm]

@jmini - we didn't discuss that particular scenario, but my suggestion is to use addXYZ for adding single items into a map and use setABC to override the whole map.

[arthurdm]

Thinking a bit more, I guess it depends on what the semantics we want to have. Something like addPathItems would just append the items to the current list, whereas setPathItems would override what's current in the map with the incoming items. Do we see use for both scenarios? Or is it overcrowding?

[jmini]

In my opinion we do not need to have both:

Paths addPathItems(Map<String, PathItem> items);
void setPathItems(Map<String, PathItem> items);

This will be confusing.

---

The builder pattern make sense for the single add item, using this method:
addPathItems(String, PathItem)

See this example (from the TCK):

.paths(OASFactory.createObject(Paths.class)
    .addPathItem("/modelReader/airlines", OASFactory.createObject(PathItem.class)
        .GET(OASFactory.createObject(Operation.class)
            .summary("Retrieve all available airlines")
            .operationId("getAirlines")
            .responses(OASFactory.createObject(APIResponses.class)
                .addAPIResponse("404", OASFactory.createObject(APIResponse.class)
                    .description("No airlines found")
                    .content(OASFactory.createObject(Content.class)
                        .addMediaType("n/a", OASFactory.createObject(MediaType.class)))))))
    .addPathItem("/availabilityModel", OASFactory.createObject(PathItem.class)
        .GET(OASFactory.createObject(Operation.class)
            .tags(new ArrayList<String>())
                .addTag("Availability")
            .summary("TEST SUMMARY")
            .operationId("getTestFlights")
//...

I do not see the benefit of having a builder pattern for the Map. Even with the new builder method for Maps (java.util.Map.of(k1, v1, k2, v2) - Java 10), I think that the single add builder method is more readable.

---

My recommendation, add a simple setter like this:

void setPathItems(Map<String, PathItem> items);

If we use a setter method, then it is natural to have a void return value.

[arthurdm]

yup, agreed. Let's go with that.

[jmini]

PR for 1.1: #283
PR for 2.0: #287

[jmini]

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.