Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to disable normalization for property keys in EachProperty #11216

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

altro3
Copy link
Contributor

@altro3 altro3 commented Sep 29, 2024

In my case I faced such a problem: I created a hierarchical structure for configuring several databases, each of which had settings for several tables. I used the EachProperty annotation and did not expect that the value injected into the Parameter variable would not be the one I set, but already normalized. Thus, the names of the databases and tables were converted, although I did not need this.

I realized that there is currently no way to disable normalization for such a case. This PR adds the normalizePropertyKeys flag to disable automatic key normalization for beans created using EachProperty

@altro3
Copy link
Contributor Author

altro3 commented Sep 29, 2024

@dstepanov @graemerocher look to this, pls

@altro3 altro3 force-pushed the each-property branch 2 times, most recently from a99e718 to e04d78d Compare September 29, 2024 12:52
@dstepanov
Copy link
Contributor

I think the correct way would be to have the catalog option to the EachProperty annotation and use it during the fetching of the properties

@sdelamo
Copy link
Contributor

sdelamo commented Sep 30, 2024

In my case I faced such a problem: I created a hierarchical structure for configuring several databases, each of which had settings for several tables. I used the EachProperty annotation and did not expect that the value injected into the Parameter variable would not be the one I set, but already normalized. Thus, the names of the databases and tables were converted, although I did not need this.

I realized that there is currently no way to disable normalization for such a case. This PR adds the normalizePropertyKeys flag to disable automatic key normalization for beans created using EachProperty

I don't not think you should used the bean name, the @EachProperty @parameter variable, as the database name. You should create a different property in your configuration object.

@EachProperty("app.tables")
public class TableProps {

    private String beanName;
    private String name;
    private String url;
    private int serverPort;

    public TableProps(@Parameter String beanName) {
        this.name = name;
    }

@altro3
Copy link
Contributor Author

altro3 commented Sep 30, 2024

Yes, of course you can duplicate the name in a separate field, as you suggest, but when you have more than 30 databases with 2-3 tables, you try to optimize the configs and (most importantly!) avoid duplicating values, because it is very difficult to find an error in such a config later.

In my opinion, normalization is good. BUT! It should not be forced, i.e. there should always be a way to use variable names or keys for the map generated by eachProperty "as is".

Recently there was a request to do something similar, regarding other methods: #11137

@sdelamo
Copy link
Contributor

sdelamo commented Oct 2, 2024

Yes, of course you can duplicate the name in a separate field, as you suggest, but when you have more than 30 databases with 2-3 tables, you try to optimize the configs and (most importantly!) avoid duplicating values, because it is very difficult to find an error in such a config later.

In my opinion, normalization is good. BUT! It should not be forced, i.e. there should always be a way to use variable names or keys for the map generated by eachProperty "as is".

Recently there was a request to do something similar, regarding other methods: #11137

For me, the issue is not duplication. The issue is you are using the same value for different things, such as the bean name and database name, which is prone to errors.

@altro3
Copy link
Contributor Author

altro3 commented Oct 2, 2024

Everything is correct, one value for two variables. I am not embarrassed that the beans will have names not in the kebab case. The problem is that I cannot set the names of such beans either. That is, I believe that the best solution is to give the user the ability to manage behavior in this situation and not make strict rules for normalization.

It all comes from the fact that in this context, in fact, the name of the bean does not matter to me, because I manage the configuration not by the names of the beans, and the inject occurs as a map. And all I want is just for the micronaut not to modify my configuration, if this is a conscious decision on my part. Therefore, be that as it may, my opinion remains the same: a tool is needed to disable forced normalization.

I like Denis's @dstepanov solution, I will try to remake this PR in the near future so that it is possible to disable normalization at the EachProperty annotation level

@graemerocher
Copy link
Contributor

I don't believe we can include this. Enabling this will break many things that depend on this normalisation. It might work for your isolated case but not generally and will just lead users down the wrong path.

@graemerocher
Copy link
Contributor

As an alternative if you must have this change is adding a skipNormalizationForPrefixes method that allows to specify a limited set of prefixes that you want to skip this normalisation for. That would be an acceptable change.

@altro3
Copy link
Contributor Author

altro3 commented Oct 3, 2024

As I said earlier, I liked Denis's suggestion. I implemented it. All that changed was the ability to specify a catalog in the EachProperty annotation. I think this solution is very good and allows you to disable normalization consciously and only for certain subsets of properties.

@dstepanov @sdelamo @graemerocher Please, review it

@altro3 altro3 force-pushed the each-property branch 2 times, most recently from f3366d8 to 9bbf84d Compare October 3, 2024 07:13
* @since 4.7.0
*/
@NonNull
default Collection<String> getPropertyEntries(@NonNull String name, PropertyCatalog propertyCatalog) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add nullability annotation to second parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

/**
* The property catalog to use.
*/
protected enum PropertyCatalog {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a breaking change for classes that subclass this class. Deprecate using forRemoval instead.

Copy link
Contributor Author

@altro3 altro3 Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why it's breaking change? This enum was internal, not public api. I just moved it to public API.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't appear to be annotated with @Internal

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not annotated, but it has a scope of protected and cannot be used from outside.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code suggests otherwise:

package example;

import io.micronaut.context.env.PropertySourcePropertyResolver;

public class TestResolver
    extends PropertySourcePropertyResolver {
    public TestResolver() {
        super();
        PropertyCatalog[] values = PropertyCatalog.values();
        System.out.println("values = " + values);
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out that I can't use PropertyCatalog in an annotation and I need to create a second PropertyCatalog enum that will be used specifically in the annotation. And do mapping between them. Right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or duplicate them. We follow semantic versioning which prohibits breaking changes in a minor release so we cannot merge changes that may break user code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I did everything, now the changes will be invisible

Copy link
Contributor Author

@altro3 altro3 Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{D7CF0F89-FD03-45D4-BBE3-EE9B393BFEFF}

@graemerocher graemerocher changed the title Add ability to disable normalization for property keys Add ability to disable normalization for property keys in EachProperty Oct 3, 2024
@graemerocher graemerocher added the type: enhancement New feature or request label Oct 3, 2024
@altro3 altro3 force-pushed the each-property branch 2 times, most recently from 4f90ae7 to bd38616 Compare October 3, 2024 15:03
@sdelamo sdelamo added this to the 4.7.0 milestone Oct 7, 2024
@sdelamo sdelamo merged commit b020fb6 into micronaut-projects:4.7.x Oct 9, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants