Key of an element should be exposed in the element

[jmini]

Originally posted as Question 2 in #234

---

When you work a lot with the model classes, for each member of a map (like PathItem element in the Paths object), you need to transport the key and the value everywhere because the information you need in contained in Entry<String, PathItem>.

Example:

void printOperationsInfo(String path, PathItem path);

Or to have your own holder class:

static class PathItemWithPath {
    final String pathString;
    final PathItem path;

    public PathItemWithPath(final String pathString, final PathItem path) {
        super();
        this.pathString = pathString;
        this.path = path;
    }

---

The Kaizen-Parser Model did solve this with:

	String getPathString();

(source)

Maybe the wording is not ideal, @arthurdm pointed that the API needs to be consistent. PathItems are added with addPathItem(String, PathItem):

microprofile-open-api/api/src/main/java/org/eclipse/microprofile/openapi/models/Paths.java

Lines 34 to 41 in 8e0f247

	/**
	* Adds the given path item to this Paths and return this instance of Paths
	*
	* @param name a path name in the format valid for a Paths object. The field name MUST begin with a slash.
	* @param item the path item added to the list of paths
	* @return the current Paths instance
	*/
	Paths addPathItem(String name, PathItem item);

So the getter for the key in the owner map in PathItem should be called getName().

---

This issue needs further discussion and checks.
Other requirement having switched to immutable maps (see #240).

Then we can add also TCK tests that ensure that when you mutate a map the exposed key is also updated.

[arthurdm]

+1. I think this is an important improvement to the usability of these models.

I like Kaizen's approach of calling this a PathString, which ore accurately describes what it is. What if we used getPathString, and then for consistency we just rename the input field of the related methods from name to pathString, such as Paths addPathItem(String pathString, PathItem item);?

[arthurdm]

Discussed in Sept 10 hangout. +1 to this proposal. Most places will still use name, but in cases where it makes sense to have another name we should change it (i.e. pathString)

[jmini]

Thank you for the transcript of the hangout.

For me the feature only makes sense when we have immutable maps (#240), otherwise the map content can be manipulated, without updating the name inside the value.

Example on Paths (assuming that the key is changed to pathString):
when Paths addPathItem(String pathString, PathItem item); is called if you later get the PathItem instance with the method PathItem getPathItem(String pathString); then item.getPathString() should return the same key.

With version 1.1 I do not see how this can be implemented. We need #248 to be solved completely. Meaning we need version 2.0

[arthurdm]

Discussed in the hangout today. One approach is to override the methods that add a single item to the map (in 1.1) so that we can keep the key at the map level consistent with the key at the child object level.

[wojtczat]

@arthurdm I'll work on this issue

[jmini]

One approach is to override the methods that add a single item to the map (in 1.1) so that we can keep the key at the map level consistent with the key at the child object level.

The problem is that in 1.1 this is not the single way of mutating maps. Even the TCK also checks that adding an element with the put(K,V) method is possible:

microprofile-open-api/tck/src/main/java/org/eclipse/microprofile/openapi/tck/ModelConstructionTest.java

Lines 378 to 379 in 14590bc

	final PathItem pathItemValue2 = createConstructibleInstance(PathItem.class);
	assertNull(p.put(pathItemKey2, pathItemValue2), "No previous mapping expected.");

With 2.0 this approach is definitively possible.

---

An other aspect that should be considered / ensured: if a PathItem was already added somewhere (as value for specific key in the Paths map or as value for a specific key in the Callback element), it should not be possible to add it to an other map.

[jmini]

@wojtczat: Nice to see your PR #290 merged.

For consistency reason, as @arthurdm suggested the parameter name in the addPathItem(String, PathItem) methods should be also renamed to pathString.

In Paths:

microprofile-open-api/api/src/main/java/org/eclipse/microprofile/openapi/models/Paths.java

Lines 34 to 41 in 8c8c649

	/**
	* Adds the given path item to this Paths and return this instance of Paths
	*
	* @param name a path name in the format valid for a Paths object. The field name MUST begin with a slash.
	* @param item the path item added to the list of paths
	* @return the current Paths instance
	*/
	Paths addPathItem(String name, PathItem item);

In Callback:

microprofile-open-api/api/src/main/java/org/eclipse/microprofile/openapi/models/callbacks/Callback.java

Lines 38 to 49 in 8c8c649

	/**
	* Adds the given PathItem to this Callback's list of PathItems using the string as its key.
	*
	* The key that identifies the Path Item Object is a runtime expression that can be evaluated in the context of a runtime HTTP request/response to
	* identify the URL to be used for the callback request. A simple example might be $request.body#/url. However, using a runtime expression the
	* complete HTTP message can be accessed. This includes accessing any part of a body that a JSON Pointer RFC6901 can reference.
	*
	* @param name a runtime expression that can be evaluated in the context of a runtime HTTP request/response
	* @param pathItem a path to add to this Callback's list of PathItems
	* @return the current Callback instance
	*/
	Callback addPathItem(String name, PathItem pathItem);

@wojtczat: Do you want to work on this?

---

More generally, I still think that this change belongs to 2.0 and not 1.1. I think the change should be cherry-picked on the 2.0 branch and reverted from master.

But I am not really impacted by what happen for the 1.1 version.

[arthurdm]

funny, I was thinking about this the other day. =) In theory I guess it is a breaking change since someone's model implementation would not compile. Unless we put a default impl returning "", but that's strange.

[wojtczat]

Yeah sure I'll work on that @jmini

[jmini]

@arthurdm:

In theory I guess it is a breaking change since someone's model implementation would not compile.

from a java API perspective, I think it is allowed to add a method. I think that breaking changes is from an API user perspective. We did a lot of method addition (all the methods that were renamed. For for 1.1 this correspond to: addition of a new method + deprecation of the old one). The vendors will need to adjust their implementation, this is not a problem.

For me the main issue with having this method in 1.1 is that the addition of element in a map can not be controlled (if added with the addPathItem(String, PathItem) you can set the value you want to return with getPathString(), if added on the map getPathItems().put(String, PathItem) then it this not possible)

[arthurdm]

I think one of the things we missed is a setPathString method in PathItem. For 1.1 we can then add a Javadoc to that method saying that its value is meant to always match the key used when inserting this PathItem into a Map. Setting it to anything else, or not setting it, is a user error.

So for now, make the user of the model responsible for setting that, since we cannot control what they can do with the inherited Map mutations.

Thoughts?

[jmini]

I think one of the things we missed is a setPathString(String) method in PathItem.

I do not think that this method necessary at API level.

1. The KaiZen model is not providing a setter in their Path interface

2. In my opinion this will make the model more complex than necessary. For the moment we have more or less POJO. If we provide a setter for that is modifying the key of the element in the corresponding map, this means that the Object needs to keep track of the map it is included in. Then when a value is provided, it needs to check if the key exist in the map or not, and if not it needs to remove itself from the map and add itself as value for the provided key in the setter.

What is the use-case for this setter?

I think we should start by adding the getter as described in this issue. If there is a need for a getter we might add it as well, but right now I do not see the value compared to the complexity of implementation.

There is still work to do to finish it:

• Renaming of the keys
• TCK tests (the most important part in my opinion)

And then extend the pattern we are discussing to other elements in other maps.

For 1.1 we can then add a Javadoc to that method saying that its value is meant to always match the key used when inserting this PathItem into a Map. Setting it to anything else, or not setting it, is a user error.

When I wrote this issue, I was more thinking that the user has nothing to set. He can access the value in order to not have to work with Map.Entry<String, PathItem> when he is iterating over the paths map.
This is the main use-case. From my work in OpenAPI generator, I know it is useful (because this is a pattern you see again and again).

[arthurdm]

I was thinking from a vendor's implementation side: once a new PathItem is inserted into the Map I would like to set its path string field to match the key used - but if I am only working with the interface (https://github.com/eclipse/microprofile-open-api/blob/master/api/src/main/java/org/eclipse/microprofile/openapi/models/PathItem.java) then I don't currently have a way to set that field.

[jmini]

I was thinking from a vendor's implementation side

I am pretty new to this API / vendor stuff, but isn't the API intended to be used by users of the lib? If some vendors need a setter they can add it, but it should not be exposed at API level to the user.

[arthurdm]

yup, it is indeed user-centric. But, if there are things we can do to make it easier to implement the spec (for a vendor), so that they don't have to cast into impls, then that would be good too.

I am ok either way with this - just wanted to bring up that vendors will have to cast (i.e. ((PathItemImpl)pathItem).setPathString(myString)). So if they have different possible implementations, they would have to check for instanceOf first.

[jmini]

In consideration of the upcoming MicroProfile OpenAPI 1.1 release scheduled for mid Dec (see #296), I think that the feature is not yet ready to be included in 1.1.
This is one other reason why we should target this feature for 2.0.

Action:
Revert the commit introduced by #290 on our master branch and continue to work on this on the 2.0 branch.

[arthurdm]

that sounds ok with me. @jmini - if you wanted to do the revert, I am fine with this only going into 2.0.

[MikeEdgar]

We discussed this issue in today's meeting and the current thinking is that having adding this functionality may limit the re-usability of the value models and also introduce some challenges with keeping the things in sync. @jmini, is this something you still have an interest in pursuing/discussing further?