Unify label handling in adapters & pipeline elements

[bossenti]

Currently, we have two approaches to define and manage labels in StreamPipes.

Option A: define labels as static strings in a class, e.g.:

public class MqttKeys {
  public static final String AUTH_MODE = "auth-mode";
  public static final String BASIC_AUTH_ALTERNATIVE = "username-alternative";
  ...
}

Option B: define an enum with label keys, e.g.:

public enum OpcUaLabels {
    OPC_HOST_OR_URL,
    OPC_URL,
    ...
}    

I personally would prefer to use option B as recommended development pattern.
One downside this approach comes with is the more verbose accessing of values, e.g.:

Alternatives.from(Labels.withId(OpcUaLabels.UNAUTHENTICATED.name()))

This can be overcome by implementing a new signature of withId that takes an Enum value as input and calls the .name() method.

The resource file would then use the same key as the enum value is called, e.g.:

OPC_HOST_OR_URL.title=OPC Server
OPC_HOST_OR_URL.description=

In my opinion this is more concise and consistent.

We could agree on using the enum approach as our recommended way of handling label keys but still keep the old way so that we don't break anything.

How do you think about this proposal? Is there anything more to consider?

[tenthe]

Hello @bossenti,
I totally agree with you and would also prefer option B.
I have already integrated the new interface that you suggested public static Label withId(Enum<?> internalId) in the branch I am working on.
My suggestion would be that we deprecate the Label.withId(String internalId) method, but not remove it right away.

Unfortunately, the Label class is not the only class where we need to specify an internalId. This is needed at several interfaces (e.g. IParameterExtractor). Since this is a major refactoring, I suggest creating a separate issue for it.
I think we can still use the new convention, a developer just needs to call the .name() method on the enum for any interfaces that have not yet migrated.

Another thought regarding changing variable names. When we use enums, the convention will be uppercase. However, many adapaters, processors, or sinks use lowercase. When we migrate them, we need a way to migrate this for existing instances as well. Does anyone have any ideas on this? Do we need to update the descriptions in couchdb for this or is it enough to replace the assets in the .streampipes directory?

Cheers,
Philipp

[bossenti]

I totally agree with you and would also prefer option B.
I have already integrated the new interface that you suggested public static Label withId(Enum<?> internalId) in the branch I am working on.
My suggestion would be that we deprecate the Label.withId(String internalId) method, but not remove it right away.

I already saw the related PR (#1665), so we can take this one as a reference implementation.
I really like this style 🤩

[bossenti]

Unfortunately, the Label class is not the only class where we need to specify an internalId. This is needed at several interfaces (e.g. IParameterExtractor). Since this is a major refactoring, I suggest creating a separate issue for it.
I think we can still use the new convention, a developer just needs to call the .name() method on the enum for any interfaces that have not yet migrated.

Thats a good point. Thanks for bringing this up, I'll prepare an issue for this.

[bossenti]

Here we go: #1669

[bossenti]

When we migrate them, we need a way to migrate this for existing instances as well. Does anyone have any ideas on this? Do we need to update the descriptions in couchdb for this or is it enough to replace the assets in the .streampipes directory?

I cannot add a lot here, but doing both should bring us on the safe side?

[dominikriemer]

Hi,
in the beginning I thought that's a good approach since it's a bit more concise, but I'm unsure if we should do a change here due to the following reasons:

• It would cause (severe) migration issues since, once the ids of configurations change, they would also need to be updated in all existing pipelines and adapters (in the onPipelineStarted, old-style IDs are arriving which cannot be handled by the processor) and I don't know a good way to automatically migrate the IDs since there's no clear mapping.
• Using capital letters as keys in property files is rather uncommon (see https://en.wikipedia.org/wiki/.properties) and it might be confusing when mixing this structure with internationalization frameworks in the future.

So due to the rather small improvements and these drawbacks, I'm probably more in favor to leave the approach as-is and only change or deprecate the structure of our developer-faced APIs in case it's really needed.

What do you think?

[bossenti]

Thanks a lot for your thoughs @dominikriemer!
These are valid points.

[bossenti]

Regarding the structure of .properties files:

Unlike many popular file formats, there is no RFC for .properties files and specification documents are not always clear, most likely due to the simplicity of the format.

I think the examples on Wikipedia are simply because camelCase is the most widely used in the Java ecosystem.

Are there any issues you can think of in terms of internationalization?
Naively, I would say that with the proposed style, this should not be a problem, since you can easily pre or post the language.
But then that brings us to your first remark, where I agree with @tenthe and think we should find a solution in general.

[dominikriemer]

Absolutely, proper migration is definitely the bigger issue. My other point is more related to developer experience, although it's not a specification, I've never seen a property/configuration file with all-caps keys and I'm asking myself if this could be confusing for Java developers who start implementing pipeline elements?

[tenthe]

Hello,
I think regardless of the solution we choose, we should document it somewhere for developers.

Regarding the case sensitivity, I have no opinion.

However, the point about updatability is very good.
This is something we need a solution for, because it can easily happen that labels change.
That can also happen regardless of the changes from this discussion.

What are the options that we have?