[Refactoring] Aggregate widely used constants.

[Alfusainey]

There are important constants in the cloud event API that are widely used throughout the SDK. Such constants should reside in one place and be properly documented (possibly adding references to the respective section of the spec).

This patch attempts to put all constants for property names and prefixes for protocol bindings in one place.

Signed-off-by: Alfusainey Jallow alf.jallow@gmail.com

[Alfusainey]

@slinkydeveloper i hope i didnt break any contributing guidelines by providing a patch without first filling an issue?

[slinkydeveloper]

@Alfusainey no problems, although I'm not 100% comfortable in having the "protocol binding constants" in api. This goes a bit against the purpose of that package.

Also I don't see the need of "global constants" for attribute names (since they are spec version dependant), nor for yet another cloudevents version constant, since we already have the specversion enum

[slinkydeveloper]

Even though there are obvious "repetitions" throughout different spec versions, I think it's weird to have a Constants class where we mix 0.3 and 1.0 constants. For example, DATASCHEMA and SCHEMAURL: these are semantically the same field, but one is the name for the 0.3 and one is the name for 1.0, so to me is confusing to have a constant for DATASCHEMA and SCHEMAURL in the same place

[Alfusainey]

Even though there are obvious "repetitions" throughout different spec versions, I think it's weird to have a Constants class where we mix 0.3 and 1.0 constants. For example, DATASCHEMA and SCHEMAURL: these are semantically the same field, but one is the name for the 0.3 and one is the name for 1.0, so to me is confusing to have a constant for DATASCHEMA and SCHEMAURL in the same place

yes, i see your point. i try to disambiguate that using the comments for each attribute name.

i see your other point too regarding the Specversion enum. Can this be used then instead of the Constants?

[slinkydeveloper]

@Alfusainey What you want to place inside SpecVersion enum that isn't already there?

[Alfusainey]

@Alfusainey What you want to place inside SpecVersion enum that isn't already there?

no I didnt mean adding anything in that enum. my question was whether we can in fact use the SpecVersion enum to replace the string literal attribute names instead of using the Constants class in this patch

[slinkydeveloper]

no I didnt mean adding anything in that enum. my question was whether we can in fact use the SpecVersion enum to replace the string literal attribute names instead of using the Constants class in this patch

You can do SpecVersion::V1.toString()

[Alfusainey]

no I didnt mean adding anything in that enum. my question was whether we can in fact use the SpecVersion enum to replace the string literal attribute names instead of using the Constants class in this patch

You can do SpecVersion::V1.toString()

i think i should be more clear :-) i introduced the constants class so that occurrence of "source" can be replaced with Constants.ATTRIBUTE_SOURCE, and "datacontenttype" with Constants.ATTRIBUTE_DATACONTENTTYPE, etc etc.

Now if i understand your previous message correctly, you said that since we already have a SpecVersion enum, defining a dedicated Constants class is not needed since the constants for the attribute names are defined in the SpecVersion enum. So this means that we can replace "source", "datacontenttype", etc string literals in the code with its corresponding constant defined in SpecVersion enum, right?

[slinkydeveloper]

@Alfusainey No I was talking in particular about these two https://github.com/cloudevents/sdk-java/pull/282/files#diff-55e4715e8931e785311e85867a858e9516b09158e4a10d16a1285eb9a17a1a79R30

Ok so I was looking at this PR again and these are my 2 cents about it:

• First of all, in Constants there are variables related to protocol bindings, we should remove these because the api module doesn't have any knowledge of protocol bindings.
• AFAIK Constants increases the size of the bytecode, because even if it just contains constants, it's still a class definition, hence bytecode is produced for it. Moving to SpecVersion removes the need to have another class.
• There is a tradeoff we need to consider between DRY, thanks to these constants, and the problem of supporting them. One day, when we'll add a new specversion (and we'll remove an old one), these variables will be part of the "breaking surface" of the sdk. Smaller the breaking surface, happier the user.
• Do we value these constants useful outside the sdk? There are some use cases where the user might need them?
• Even if we move all these constants in SpecVersion, we're still mixing v1 and v0.3 constants in the same class, that is SpecVersion.ATTRIBUTE_DATASCHEMA and SpecVersion.ATTRIBUTE_SCHEMAURL. A non experienced user of CloudEvents might easily pick the wrong one.

Does these concerns make any sense?

[pierDipi]

I think having some constants for attribute names is useful.

First of all, in Constants there are variables related to protocol bindings, we should remove these because the api module doesn't have any knowledge of protocol bindings.

Even if we move all these constants in SpecVersion, we're still mixing v1 and v0.3 constants in the same class, that is SpecVersion.ATTRIBUTE_DATASCHEMA and SpecVersion.ATTRIBUTE_SCHEMAURL. A non-experienced user of CloudEvents might easily pick the wrong one.

I agree, if we want them, constants should be versioned and protocol agnostic.

AFAIK Constants increases the size of the bytecode, because even if it just contains constants, it's still a class definition, hence bytecode is produced for it. Moving to SpecVersion removes the need to have another class.

Do we have a measure of how much bigger?

Do we value these constants useful outside the sdk? There are some use cases where the user might need them?

Yes, we need them in eventing-kafka-broker, and we're using ContextAttributes enums to get them which aren't meant to be used how we use them though.

[slinkydeveloper]

Yes, we need them in eventing-kafka-broker, and we're using ContextAttributes enums to get them which aren't meant to be used how we use them though.

yeah and that's why i'm removing these classes, they're quite wrong and they do nothing more than a set of public final static constants can't do...

I wonder, since the #280 adds these constants inside CloudEventV[x] classes, does it makes sense to just have these public? https://github.com/cloudevents/sdk-java/pull/280/files#diff-e54907532f1e43b42939366f4bf9b722af6f4bd66de0faac5659d30da5f45998R39

[pierDipi]

I think we agree :), see review in #280.

In this PR or another one, we can refactor the code to remove hardcoded strings in various integrations using #280 (if any).

[Alfusainey]

• First of all, in Constants there are variables related to protocol bindings, we should remove these because the api module doesn't have any knowledge of protocol bindings

I see your point! The reason why i added them in the api is that the BaseGenericBinaryMessageReaderImpl#isCloudEventsHeader(HK) is pretty much a duplicated method in all the (current) supported protocol bindings. The only difference is that the prefix name is unique for each protocol. Thus the isCloudEventsHeader(HK) can be implemented in the base class itself as a template method. Now each protocol binding will implement (say getProtocolPrefix()) to return their specific protocol prefixes using Constants.CE_PREFIX_AMQP in the case of AMQP. But yes, these constants can definitely go into the specific protocol impl modules.

AFAIK Constants increases the size of the bytecode, because even if it just contains constants, it's still a class definition, hence bytecode is produced for it. Moving to SpecVersion removes the need to have another class

ok, i was not thinking about this. i just wanted a way to use attribute names and having them defined (and documented) once somewhere that everyone can use would be great. that was my only reasoning for this PR

There is a tradeoff we need to consider between DRY, thanks to these constants, and the problem of supporting them. One day, when we'll add a new specversion (and we'll remove an old one), these variables will be part of the "breaking surface" of the sdk. Smaller the breaking surface, happier the user.

thats true 👍

Do we value these constants useful outside the sdk? There are some use cases where the user might need them?

i think @pierDipi already answered this. if you see the javadoc at the class level, i mentioned that the class contains constants used throughtout the SDK. so i was only focused on the sdk itself

Even if we move all these constants in SpecVersion, we're still mixing v1 and v0.3 constants in the same class, that is SpecVersion.ATTRIBUTE_DATASCHEMA and SpecVersion.ATTRIBUTE_SCHEMAURL. A non experienced user of CloudEvents might easily pick the wrong one

I see.. this is like putting salt and sugar at the same location in a kitchen :-) however, if the salt and the sugar are properly labeled (i.e well documented using javadoc comments), then i expect a user to first read the label before putting salt in his tee. I feel the same way with constants that are properly documented

I just took a look at #280. i think if we make those attribute names public (and well documented), then we do not need this patch at all since it would already be taken care of in #280

@slinkydeveloper @pierDipi given that there is a consensus here, can i close this patch?

[Alfusainey]

In this PR or another one, we can refactor the code to remove hardcoded strings in various integrations using #280 (if any).

right, thats also an idea. that would then first require #280 to be merged first. ok, i can just leave this PR open for now

[slinkydeveloper]

#280 merged

[Alfusainey]

#280 merged

PR updated to reuse constants addded in #280

[slinkydeveloper]

Thanks for your contribution!