Wrong generated metadata for properties with successive capital letters

[mbogoevici]

Related to spring-cloud/spring-cloud-stream#396

This could be either an issue for relaxed binding or metadata generation.

Given a property such as: spring.cloud.stream.binder.rabbit.default.autoBindDLQ the generated metadata will be:

{
  "name": "spring.cloud.stream.binder.rabbit.default.auto-bind-dlq",
  "type": "java.lang.Boolean",
  "sourceType": "org.springframework.cloud.stream.binder.rabbit.config.RabbitBinderConfigurationProperties",
  "defaultValue": false
}

However, spring.cloud.stream.binder.rabbit.default.auto-bind-dlq does not map back to spring.cloud.stream.binder.rabbit.default.autoBindDLQ (it maps to spring.cloud.stream.binder.rabbit.default.autoBindDlq instead).

[snicoll]

I don't think the title is accurate. The meta-data is correct (our canonical representation is lower case hyphen, we're not going to go upper case just because the key has several upper case letters in a row).

[wilkinsona]

We've come to realise that the relaxed binding is too relaxed. We plan to fix that in 2.0 (see #4910). With one eye on that goal, we should not change the relaxed binding so that auto-bind-dlq can be bound to setAutoBindDLQ.

Part of #4910 will be to formalise the rules for relaxed binding so that they're easy to describe and implement. One such rule will probably be that letters preceded by a separator are capitalised and the separator is removed. Given our supported separators, - and _, this would mean that autoBindDLQ, auto_bind_d_l_q and auto-bind-d-l-q all bind to setAutoBindDLQ. In fact, all three of those options are supported by the binder today.

I think that we should address this issue by changing the metadata that's generated. The convention for the metadata is kebab-case so it should be auto-bind-d-l-q. If -d-l-q is considered to be ugly, the property should be renamed to setAutoBindDlq. This would allow autoBindDlq, auto-bind-dlq, and auto_bind_dlq to be used, without making the relaxed binding rules overly complicated.

[garyrussell]

In fact, all three of those options are supported by the binder today.

I believe I tested it with auto-bind-d-l-q yesterday and it still didn't work. I can try again if needed.

[mbogoevici]

I still believe that the title is correct, in the sense that given a property named autoBindDLQ, the generated metadata shouldn't be auto-bind-dlq, since using that as a hint will not get the property populated at runtime.

[mbogoevici]

Happy to change the property name to work around this, but that doesn't change the fact that the currently generated metadata is incorrect.

[wilkinsona]

@mbogoevici Stephane's already changed the title to accurately describe the problem with the metadata

[wilkinsona]

@garyrussell Thanks, you're right. When I checked, I did this:

for (String name: new RelaxedNames("auto-bind-d-l-q")) {
    System.out.println(name);
}

It produces autoBindDLQ. Unfortunately, that's not the call that the implementation makes. It actually passes in auto-bind-dLQ having applied a camel case to hyphen conversion to autoBindDLQ. So it's going the other way and with different (and slightly odd) input.

In short, this is another example of why we want to simplify this and harden up the rules. Hopefully we can fix the binding too in 1.3.4 without breaking anything else.

[snicoll]

Happy to change the property name to work around this

I wouldn't want to see a suggestion with auto-bind-d-l-q so, yes please!

[mbogoevici]

@snicoll

I wouldn't want to see a suggestion with auto-bind-d-l-q

Me neither, but thought of raising an issue around the original problem we encountered. Thanks for the turnaround.

[dsyer]

Doesn't feel to me like this one was resolved. We dodged it by changing the property name in Spring Cloud Stream, but that can't last for ever (as evidenced by the eureka configuration issue).

[wilkinsona]

@dsyer The issue's still open.

Aesthetics aside, I think the metadata needs to change: autoBindDLQ should produce auto-bind-d-l-q in the metadata. There's then a secondary problem as the relaxed name support doesn't work. It converts autoBindDLQ to auto-bind-dLQ and then generates relaxed names from that.

@snicoll tells me that fixing the metadata is trivial. I need to spend some more time looking at the other half of the problem to be more certain that we can fix it without breaking something else. In other words, there's a chance that we'll need to wait till 2.0 to really fix this.

[snicoll]

@snicoll tells me that fixing the metadata is trivial.

Yup, we're actually transforming the original form (and that was the wrong call, sorry about that). See ItemMetadata#buildName and #2118

[wilkinsona]

We need to devise some simple (to implement and to explain) rules for the binding. The simplest rule that I can think of is that any upper-case letter is prefixed with a separator and converted to lower-case. This works well for camel-cased properties, but is a little ugly when there are successive upper-case letters:

Input	Output
autoBindDlq	auto-bind-dlq
autoBindDLQ	auto-bind-d-l-q
eurekaServerDnsName	eureka-server-dns-name
eurekaServerDNSName	eureka-server-d-n-s-name

A slightly more complex rule would be that any upper-case letter is prefixed with a separator unless the following letter is also upper-case, all upper-case letters are converted to lower-case:

Input	Output
autoBindDlq	auto-bind-dlq
autoBindDLQ	auto-bind-dlq
eurekaServerDnsName	eureka-server-dns-name
eurekaServerDNSName	eureka-server-dns-name

[philwebb]

My vote is option 2

[wilkinsona]

Either I was wrong above or another change has improved this. As far as I can tell, auto-bind-d-l-q works. I've added an equivalent test in 9d194c2 to verify.

I believe that all that remains to be done (before we tackle #4910) is to fix the metadata: a property named autoBindDLQ should, with the current approach to bindings, generate auto-bind-d-l-q as its metadata.

@snicoll Over to you, I think.