casid / jte

Secure and speedy templates for Java and Kotlin.
https://jte.gg
Apache License 2.0
842 stars 63 forks source link

JTE Spring Boot properties aren't in dash casing #382

Closed mhalbritter closed 1 month ago

mhalbritter commented 1 month ago

Hello,

i've noticed that the properties in the spring-configuration-metadata.json file are in camel case (e.g. gg.jte.usePrecompiledTemplates).

If you take a look at the metadata included in Spring Boot itself, it's dash cased.

Btw, there's a plugin which automatically creates the metadata files from the property files.

Would you be open to change the properties to dash-casing (e.g. gg.jte.use-precompiled-templates) for better consistency?

casid commented 1 month ago

Hi @mhalbritter,

thanks for the feedback, and yes I have no stakes in how the spring properties are named and consistency with the Spring Boot conventions makes a lot of sense.

What's the recommended way to make breaking changes like this? For instance, it would be bad if gg.jte.usePrecompiledTemplates wouldn't be applied when users upgrade and don't read the patch notes carefully.

I could think of keeping the old properties and deprecating them for removal and to throw an exception if those are still set? But maybe there is a better way?

@atomfrede, @tschuehly would you be ok with this change, too?

atomfrede commented 1 month ago

Totally fine. We could print a warning for e.g. 1 release and ignore them in the next (or throw an exception)

tschuehly commented 1 month ago

@casid @mhalbritter This shouldn't be a breaking change as Spring Boot uses relaxed binding: https://docs.spring.io/spring-boot/reference/features/external-config.html#features.external-config.typesafe-configuration-properties.relaxed-binding

kelunik commented 1 month ago

@casid Breaking changes should only be shipped in major releases. We can likely read both versions with the new name having precedence and log a warning if the old name is used? Then drop the old path in the next major.

mhalbritter commented 1 month ago

I should have mentioned that in the first post, sorry about that. That's no breaking change. Boot itself binds gg.jte.usePrecompiledTemplates the same way as gg.jte.use-precompiled-templates. However, specifying the properties in dash-casing fixes the auto-suggestions from the IDE. So it's only a "cosmetic" change.

But for the reference, if you ever come to the situation where you need to deprecate some properties: There's a deprecation attribute on the properties in the JSON: https://docs.spring.io/spring-boot/specification/configuration-metadata/format.html#appendix.configuration-metadata.format.property

casid commented 1 month ago

Thanks for the update and clarification @mhalbritter. @tschuehly already made the changes, so this will be part of the next release :-)

mhalbritter commented 1 month ago

Great! Thanks a lot!