cloudevents/sdk-java

Remove Clients' Dependency on Concrete Implementations of EventFormat

skepticoitusInteruptus opened this issue · 6 comments

Context

The intent of Java™ SE's Service Provider Interface (SPI) mechanism is to facilitate OO best practice. The value the SPI facility is meant to add to the design of a Java™ library, is to decouple clients from concrete implementations of services.

I ❤️ well-designed libraries like Java SDK for Cloud Events that leverage SPI to follow OO best practice:

  1. Program to an interface; not to an implementation
  2. Open for extension; closed for modification
  3. etc.

The Issue

In the usage example [1], the import io.cloudevents.jackson.JsonFormat of the concrete implementation of EventFormat effectively defeats the purpose of using the SPI service abstraction facility.

Current Design

import io.cloudevents.CloudEvent;
import io.cloudevents.core.format.EventFormatProvider;
import io.cloudevents.core.builder.CloudEventBuilder;
import io.cloudevents.jackson.JsonFormat; // Tightly couples this client to a concrete implementation

CloudEvent event = CloudEventBuilder.v1()
    .withId("hello")
    .withType("example.vertx")
    .withSource(URI.create("http://localhost"))
    .build();

byte[]serialized = EventFormatProvider
    .getInstance()                          // Since clients need the concrete impl in spite of SPI, why not do this: 
    .resolveFormat(JsonFormat.CONTENT_TYPE) // var json = new JsonFormat(); 
    .serialize(event);                      // var serialized = json.serialize(event);

Proposed Enhancement

import io.cloudevents.CloudEvent;
import io.cloudevents.core.format.EventFormatProvider;
import io.cloudevents.core.builder.CloudEventBuilder;
import io.cloudevents.core.format.ContentTypes; // Introduce constant to decouple clients from implementations

CloudEvent event = CloudEventBuilder.v1()
    .withId("hello")
    .withType("example.vertx")
    .withSource(URI.create("http://localhost"))
    .build();

byte[] serialized = EventFormatProvider
    .getInstance()
    .resolveFormat(ContentTypes.JSON;) // No longer coupled to concrete io.cloudevents.jackson.JsonFormat
    .serialize(event);

——

[1] https://bit.ly/CEJsonUsgEg

Thanks for the suggestion @skepticoitusInteruptus, feel free to propose a PR.

cc @JemDay in case you have opinions on the proposed approach

Technically this is already possible with passing a string but I see how a constant in the core library might facilitate that

I'd be in favor of cleanly separating these constructs, having a core 'ContentTypes' construct makes perfect sense to me.

...Thanks for the suggestion...

Don't mention it @pierDipi 👍

...Technically this is already possible with passing a string...

It occurred to me that an enum would be more type safe might be another option to consider.

...cc @JemDay in case you have opinions on the proposed approach...

I'd be curious is there's any advantage to using an enum; I suspect most of the usage will be string based - resolving the format during message reception based on a transport header, and populating the transport header during message sending.

I guess that'll become clear as you work your proposal ;-)

I'd be curious is there's any advantage to using an enum;

Off the top of my head, from a performance angle enum constants would probably be my preferred option (vs String literals) as expressions in a switch statement, say.

Performance differences might or might not be significant, String constants (i.e., literals) are more prone to hard-to-notice typos though:

var contentType = receiveMessage( )
                  .body( )
                  .contentTypeAsString();

switch(contentType) {
   case "applicatoin/cloudevents+json":
      // ...
      break;
   ...
}

...vs...

var contentType = receiveMessage( )
                  .body( )
                  .contentTypeAsEnum();

switch(contentType) {
   case JSON:
      // ...
      break;
   ...
}

I wouldn't insist on enums, by the way. The above is just for the sake of folks' curiosity.