Define and verify behavior of schema extension properties

[MikeEdgar]

Closes #698

[MikeEdgar]

Draft for now. The bnd plugin does not like the overridden methods where a type parameter is specified in Schema. E.g.

T addExtension(String name, Object value) -> Schema addExtension(String name, Object value)

This may be a bug in the plugin, but we'll need to resolve this somehow.

@Azquelt before I make any TCK changes, can you please take a look at the JavaDoc changes and let me know if you catch anything that conflicts with the intent of #698 ?

[Azquelt]

How odd. Ordinarily I would trust the bnd baseline checker, but here it seems wrong.

Generally, changing a method's return type (which confusingly we are doing here, after erasure it used to return Extensible but now it returns Schema) counts as removing the old method and adding a new one which breaks binary compatibility.

However, the compiled interface seems to still include the method signature returning Extensible and testing shows that code compiled against the old interface but run against the one from this PR does still work.

Output from javap Schema.java from this PR:

Compiled from "Schema.java"
public interface org.eclipse.microprofile.openapi.models.media.Schema extends org.eclipse.microprofile.openapi.models.Extensible<org.eclipse.microprofile.openapi.models.media.Schema>, org.eclipse.microprofile.openapi.models.Constructible, org.eclipse.microprofile.openapi.models.Reference<org.eclipse.microprofile.openapi.models.media.Schema> {

...

  public default org.eclipse.microprofile.openapi.models.media.Schema extensions(java.util.Map<java.lang.String, java.lang.Object>);
  public abstract org.eclipse.microprofile.openapi.models.media.Schema addExtension(java.lang.String, java.lang.Object);
  public abstract void removeExtension(java.lang.String);
  public abstract void setExtensions(java.util.Map<java.lang.String, java.lang.Object>);
  public default boolean hasExtension(java.lang.String);
  public default java.lang.Object getExtension(java.lang.String);
  public default org.eclipse.microprofile.openapi.models.Extensible addExtension(java.lang.String, java.lang.Object);
  public default org.eclipse.microprofile.openapi.models.Extensible extensions(java.util.Map);
}

Given this, I'm unsure why the bnd baseline tool reports the method as having been removed.

It looks like the Extensible-returning methods are synthetic bridge methods.
Output of javap -v Schema.class:

...

  public default org.eclipse.microprofile.openapi.models.Extensible extensions(java.util.Map);
    descriptor: (Ljava/util/Map;)Lorg/eclipse/microprofile/openapi/models/Extensible;
    flags: (0x1041) ACC_PUBLIC, ACC_BRIDGE, ACC_SYNTHETIC
    Code:
      stack=2, locals=2, args_size=2
         0: aload_0
         1: aload_1
         2: invokeinterface #200,  2          // InterfaceMethod extensions:(Ljava/util/Map;)Lorg/eclipse/microprofile/openapi/models/media/Schema;
         7: areturn
      LineNumberTable:
        line 51: 0
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0       8     0  this   Lorg/eclipse/microprofile/openapi/models/media/Schema;
    MethodParameters:
      Name                           Flags
      <no name>                      synthetic

Possibly that's why the bnd baseline plugin ignores them?

Needs more looking into

[MikeEdgar]

The plugin supports a way to ignore particular elements for baselines earlier than a specified version (second commit). This seems like a reasonable work-around to me.

[Azquelt]

Do we still need a minor package version bump?

You could have a Schema implementation which inherits from an Extensible implementation and doesn't redefine extensions or addExtension in which case I don't think that class would implement the new method signatures.

[MikeEdgar]

Do we still need a minor package version bump?

You could have a Schema implementation which inherits from an Extensible implementation and doesn't redefine extensions or addExtension in which case I don't think that class would implement the new method signatures.

I'm not entirely sure. addExtension doesn't have a default, so wouldn't that require an implementation with the signature Schema addExtension(String name, Object value) ?

[Azquelt]

I'm not entirely sure. addExtension doesn't have a default, so wouldn't that require an implementation with the signature Schema addExtension(String name, Object value) ?

Yeah, I think so, and the previous version wouldn't require that (you could get away with Extensible addExtension(String name, Object value)) hence a minor bump.

[MikeEdgar]

The last commit adds the version bump to the package. Let me know if you spot anything incorrect with the docs and I'll try to get the TCK updates pushed later today to align with that.

[Azquelt]

Tests look good, my only concern is around whether we're correct about the baseline plugin being wrong.

Do you think we should raise a defect for that? Worst case we get an explanation of why the plugin is actually right.

[Azquelt]

If it were actually somehow a breaking change for consumers, we could just not override the extension methods on Schema and document the differences only in the class Javadoc.

[Azquelt]

Based on the comments from the bnd issue, I'm happy to approve this.