camaraproject / Commonalities

Repository to describe, develop, document and test the common guidelines and assets for CAMARA APIs
Apache License 2.0
9 stars 24 forks source link

CloudEvent type enum causes code generation problem #159

Closed akoshunyadi closed 2 months ago

akoshunyadi commented 4 months ago

Problem description As in https://github.com/camaraproject/DeviceStatus/issues/107 reported using the CloudEvent type identifier as an enum e.g. org.camaraproject.device-status.v0.roaming-status causes problems in the code generation for Java and probably other languages too. An enum can't contain either a dot or (in Java) lowercase letters.

Beside DeviceStatus also other APIs, like DeviceLocation/Geofencing or QoD are affected.

In current implementations a workaround has to be used, instead of the generated class.

Expected behavior

A possible solution would be to use strings without enums.

Alternative solution

Additional context

bigludo7 commented 4 months ago

Hello @akoshunyadi If we keep an enum but did not use dot or lowercase does it work?

I will check with my dev team because we do not have this issue in our code (@patrice-conil ).

I'm a bit uncomfortable to remove the enum because it means that the developer has to fill the right value and we will have to provide information when the type valued is incorrect.

akoshunyadi commented 4 months ago

@bigludo7 yes, without the "forbidden" characters it would work, but then we are not fully CloudEvents conform

I agreed with your concern about omitting the enum, that's why there is no perfect solution :-)

lbertz02 commented 3 months ago

You could do the following:

  1. Use a common cloud event structure whree the type property is a string and acts as a discriminator - no enum constraint.
  2. Make use of the fact that the specific Event's name could be the type value we seek.

For instance, we could have a cloudevent-common.yaml file:

CloudEventsBase:
     type: object
     properties:
          type:
               type: string
     discriminator:
          propertyname: type
...

Then in the specific API we would see

org.camaraproject.device-status.v0.Roaming-Status:
     description: ...
     allOf:
          - $ref: "<uri to cloudevent-common.yaml>#/components/schemas/CloudEventsBase`
          - ...

In this case there is no need for a map nor extra work for decode/encode and the type value populated would follow the recommended value. A common structure can be used across all APIs as well. The developer should not need to populate the type value in this case but that may still be required if the framework used does not autopopulation of unconstrained discriminators upon encode/decode.

If there is a wish to have a data structure in the API that, as a convenience, noting what events are defined it can be done but is not required. However, that would only be helpful for event subscription at this point.

The question is if the proposed naming will break the toolchain you are using?

patrice-conil commented 3 months ago

Hi @akoshunyadi, @lbertz02, @bigludo7, I'm not sure I understand the problem. I use openapi-generator successfully... it generates enum classes like this

public enum NotificationType {

  AREA_ENTERED("org.camaraproject.geofencing.v0.area-entered"),

  AREA_LEFT("org.camaraproject.geofencing.v0.area-left"),

  SUBSCRIPTION_ENDS("org.camaraproject.geofencing.v0.subscription-ends");

  private String value;

  NotificationType(String value) {
    this.value = value;
  }

    /**
     * Convert a String into String, as specified in the
     * <a href="https://download.oracle.com/otndocs/jcp/jaxrs-2_0-fr-eval-spec/index.html">See JAX RS 2.0 Specification, section 3.2, p. 12</a>
     */
    public static NotificationType fromString(String s) {
      for (NotificationType b : NotificationType.values()) {
        // using Objects.toString() to be safe if value type non-object type
        // because types like 'int' etc. will be auto-boxed
        if (java.util.Objects.toString(b.value).equals(s)) {
          return b;
        }
      }
      throw new IllegalArgumentException("Unexpected string value '" + s + "'");
    }

  @Override
  @JsonValue
  public String toString() {
    return String.valueOf(value);
  }

  @JsonCreator
  public static NotificationType fromValue(String value) {
    for (NotificationType b : NotificationType.values()) {
      if (b.value.equals(value)) {
        return b;
      }
    }
    throw new IllegalArgumentException("Unexpected value '" + value + "'");
  }
}

What generator do you use ?

maxl2287 commented 3 months ago

Hi @patrice-conil,

so the initial issue came up by myself. I use:

<groupId>org.openapitools</groupId>
<artifactId>openapi-generator-maven-plugin</artifactId>

Let's take a look how the CloudEvent-class looks like:

@Generated(value = "org.openapitools.codegen.languages.SpringCodegen", date = "2024-04-05T10:20:57.087149800+02:00[Europe/Berlin]", comments = "Generator version: 7.4.0")
public class CloudEvent {

...

  private NotificationEventType type;

 ...

  /**
   * Constructor with only required parameters
   */
  public CloudEvent(String id, String source, NotificationEventType type, SpecversionEnum specversion, OffsetDateTime time) {
    this.id = id;
    this.source = source;
    this.type = type;
    this.specversion = specversion;
    this.time = time;
  }
... 

As you can see here the constructor is expecting an Enum NotificationEventType and not the String. This means that we need to put in here (in example):

    CloudEvent cloudEvent = new CloudEvent();
    cloudEvent.setType(NotificationEventType.AREA_ENTERED);

So we are setting here the enum and not a string. When the event will be send it will then send:

{
  "id": "123655",
  "source": "https://notificationSendServer12.supertelco.com",
  "type": "AREA_ENTERED",
  "specversion": "1.0",
  "datacontenttype": "application/json",
  "time": "2023-03-22T05:40:23.682Z",
  "data": {
    ...
  }
}

But this is wrong.

Expected

The expected sending would be the value of the Enum and not the enum name. Means the String.

{
  "id": "123655",
  "source": "https://notificationSendServer12.supertelco.com",
  "type": "org.camaraproject.geofencing.v0.area-entered",
  "specversion": "1.0",
  "datacontenttype": "application/json",
  "time": "2023-03-22T05:40:23.682Z",
  "data": {
    ...
  }
}

But we cannot do:

    CloudEvent cloudEvent = new CloudEvent();
    cloudEvent.setType(NotificationEventType.AREA_ENTERED.getValue());

as this would lead to compiler-error of course.

I hope you got my point 😄

patrice-conil commented 3 months ago

Hi @patrice-conil,

so the initial issue came up by myself. I use:

<groupId>org.openapitools</groupId>
<artifactId>openapi-generator-maven-plugin</artifactId>

Let's take a look how the CloudEvent-class looks like:

@Generated(value = "org.openapitools.codegen.languages.SpringCodegen", date = "2024-04-05T10:20:57.087149800+02:00[Europe/Berlin]", comments = "Generator version: 7.4.0")
public class CloudEvent {

...

  private NotificationEventType type;

 ...

  /**
   * Constructor with only required parameters
   */
  public CloudEvent(String id, String source, NotificationEventType type, SpecversionEnum specversion, OffsetDateTime time) {
    this.id = id;
    this.source = source;
    this.type = type;
    this.specversion = specversion;
    this.time = time;
  }
... 

As you can see here the constructor is expecting an Enum NotificationEventType and not the String. This means that we need to put in here (in example):


    CloudEvent cloudEvent = new CloudEvent();
    cloudEvent.setType(NotificationEventType.AREA_ENTERED);

I think your problem is here... you need to create an instance of EventAreaEntered and let jackson/gson or whatever library you are using handle the type during serialization. Tuning the discriminator by hand is not a good idea and may end up with a double attribute.

maxl2287 commented 3 months ago

A few months ago this was not working with the generator version we used. On the first few it seems to work now.

I will check, if we can now use the Event-classes like EventAreaEntered.

Thanks @patrice-conil!

I will keep you here informed.

shilpa-padgaonkar commented 2 months ago

As agreed with @maxl2287, this issue can be closed. He will reopen the issue if something else comes up. cc_@akoshunyadi