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

Config.getValue and ConfigValue.getValue #619

Open
Emily-Jiang opened this issue Oct 6, 2020 · 3 comments
Open

Config.getValue and ConfigValue.getValue #619

Emily-Jiang opened this issue Oct 6, 2020 · 3 comments
Labels
clarification 🏆 A clarification (PR) or request for clarification (issue) in the spec or API
Milestone

Comments

@Emily-Jiang
Copy link
Member

Emily-Jiang commented Oct 6, 2020

The javadoc on Config.getValue() and ConfigValue.getVaue() are inconsistent.

config.getValue() https://github.com/eclipse/microprofile-config/blob/master/api/src/main/java/org/eclipse/microprofile/config/Config.java#L108

/**
     * Return the resolved property value with the specified type for the specified property name from the underlying
     * {@linkplain ConfigSource configuration sources}.
     * <p>
     * The configuration value is not guaranteed to be cached by the implementation, and may be expensive to compute;
     * therefore, if the returned value is intended to be frequently used, callers should consider storing rather than
     * recomputing it.
     * <p>
     * The result of this method is identical to the result of calling
     * {@code getOptionalValue(propertyName, propertyType).get()}. In particular, If the given property name or the
     * value element of this property does not exist, the {@link java.util.NoSuchElementException} is thrown. This
     * method never returns {@code null}.
     *
     * @param <T>
     *            The property type
     * @param propertyName
     *            The configuration property name
     * @param propertyType
     *            The type into which the resolved property value should get converted
     * @return the resolved property value as an instance of the requested type (not {@code null})
     * @throws IllegalArgumentException
     *             if the property cannot be converted to the specified type
     * @throws java.util.NoSuchElementException
     *             if the property is not defined or is defined as an empty string
     */


ConfigValue.getValue()

/**
     * The value of the property lookup with transformations (expanded, etc).
     *
     * @return the value of the property lookup or {@code null} if the property could not be found
     */

I think ConfigValue.getValue() should behave the same as Config.getValue(). The other thing is that ConfigValue.getValue() always return a string. Should we pass a calss type as the parameter and return the same type? If not, we should rename the method getStringValue instead.

@Emily-Jiang Emily-Jiang added the clarification 🏆 A clarification (PR) or request for clarification (issue) in the spec or API label Oct 6, 2020
@radcortez
Copy link
Contributor

Well, we had that discussion about passing the type to provide conversion, but if I remember correctly, we decided to go simple with the API and add it later. If that changed, we can add it now.

I'm not completely sure if ConfigValue should follow the same rules and getValue, mostly because ConfigValue is a container object much like Optional, so I think it should work as an Optional.

Also, the idea is that we enhance ConfigValue with additional API's and information. I think that if we introduce that restriction, we may be limiting ourselves with what we can do with it later. For instance, we could add some debugging information that details the config lookups. The config might even exist in a lower ordinal source, but emptied in a higher source. Or we can even collect all the available values in all sources for the user to decide which one to use based on some logic. If we just fail the lookup then we cannot provide such enhancements.

@Emily-Jiang
Copy link
Member Author

@radcortez Thanks for the explaination! Maybe we should just change the method getValue to getStringValue() to reflect this purpose and later on we can make getValue to return something being converted. It is good to revisit and see whether it makes us confused. Thoughts?

@radcortez
Copy link
Contributor

I guess it depends how you see it. Since the spec is all about Strings, and the original format is a String, I'm find with getValue returning a String for ConfigValue, but I don't oppose to rename it.

My idea was to add a getAs(Class) method that would perform the conversion to the specified type.

@radcortez radcortez added this to the MP Config 2.1 milestone Mar 4, 2021
@Emily-Jiang Emily-Jiang modified the milestones: MP Config 3.x, Config 4.0 Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification 🏆 A clarification (PR) or request for clarification (issue) in the spec or API
Projects
None yet
Development

No branches or pull requests

2 participants