Configuration: Move static methods to ConfigUtil. Add documentation. #5292

davidgraeff · 2018-03-21T15:03:37Z

getAllFields, as methods moved from Configuration to internal ConfigMapper
Add String->BigDecimal conversion in ConfigMapper.as
Add configuration holder object to MagicService of the magic binding to see if the mapping works for all kind of configuration values.
(Except for the case of bug core.config.ConfigurationService IllegalArgumentException with Magic binding #5291)

Signed-off-by: David Graeff david.graeff@web.de

htreu

I like the overall refactoring but I'm not sure about the new @RequiredField and @UseKey annotations: They seem not to solve a real problem we face right now. Services which are not fully configured should already log their problems and will probably not work. Using fields with different names then their counterpart in the config description is also unlikely. wdyt?

htreu · 2018-03-21T17:53:23Z

...thome.magic/src/main/java/org/eclipse/smarthome/magic/binding/internal/MagicServiceImpl.java

+    public void modified(Map<String, Object> properties) {
+        // Print changed values
+        MagicServiceConfig config = new Configuration(properties).as(MagicServiceConfig.class);
+        List<Field> l = ConfigUtil.getAllFields(MagicServiceConfig.class);


Maybe instead of exposing the reflection based method getAllFields you could give a toString to MagicServiceConfig.

True. Regarding the annotations: I actually introducted UseKey because the configuration key of one field in MagicServiceConfig where it's used, is unfortunately called "boolean", like the Java keyword.

I'm honestly a big fan of this property-map to class object mapping, partly because I do not find that powerful reflection API in many other languages. However I'd like to see this as a default way of handling service configuration and it should work in every case.

So we either need this UseKey annotation or we need to forbid certain configuration key names (Java language keywords).

htreu · 2018-03-21T18:41:14Z

For the time being I suggest to have the refactoring (and magic extension) only.

davidgraeff · 2018-03-21T20:00:40Z

Agreed. I'll open another issue for discussion. I'll rename the magic bindings configuration name from boolean to "bool".

davidgraeff · 2018-03-22T10:50:21Z

@htreu Done

davidgraeff · 2018-03-22T11:31:07Z

And I have added an issue for the annotations #5300.

htreu

Please reduce visibility to private on ConfigUtil#getAllFields.
Others are only cosmetics.

htreu · 2018-03-22T12:31:28Z

...clipse.smarthome.config.core/src/main/java/org/eclipse/smarthome/config/core/ConfigUtil.java

+     * @param clazz The class
+     * @return A list of Field objects
+     */
+    public static List<Field> getAllFields(Class<?> clazz) {


please make this private again.

And it would be nice to have the public API on top and the supporting private methods at the bottom of the class.

Yup, I usually would do that, but in this case all private/public fields and methods of the ConfigUtil class are widely mixed. If I sort them now, it would be a hard job reviewing. Should be done in another PR, if that's ok.

agreed, thats a pity.
But then please order at least the refactored code, so that public comes first, private comes last. And please make #getAllFields private.

htreu · 2018-03-22T12:35:24Z

...thome.magic/src/main/java/org/eclipse/smarthome/magic/binding/internal/MagicServiceImpl.java

+
+    @Modified
+    public void modified(Map<String, Object> properties) {
+        MagicServiceConfig config = new Configuration(properties).as(MagicServiceConfig.class);


Since you introduced the new ConfigUtil#as method you can directly use it here.

That would be the only place of all services where it would be different. I'd like to wait for the discussion and outcome of #5300, if that is ok for you.

davidgraeff · 2018-03-22T13:13:25Z

@htreu done

davidgraeff · 2018-03-22T13:49:32Z

I have sorted depending on public/private methods now. But did two more things as well:

Added a nullable to this signature: normalizeType(Object value, @Nullable ConfigDescriptionParameter configDescriptionParameter).
Deprecated normalizeType(Object value)

htreu · 2018-03-22T14:09:51Z

Deprecated normalizeType(Object value)

Why? Everything just looked so good ;-)

davidgraeff · 2018-03-22T14:22:22Z

Haha yes I know. Trouble maker /me :D
Let me explain my intention. Think of what normalizeTypes actually does: It converts all number types to BigDecimal and all collection types to Lists. Usually it would consider the configuration specification for the correct conversion. If this is not provided, it does this only on a best-effort approach.

The consumer of the API should clearly realize that he is using an API that only works best-effort and is used in a way that it originally was not intended to be used. The one argument only method hides this fact and the best way to realize the API intention is by the mandatory second argument ConfigDescriptionParameter.

htreu · 2018-03-22T14:52:17Z

Yes, I can follow your intention. Please revert the deprecation here so we can merge the refactoring. Lets discuss this new aspect on a separate PR where others can join the discussion. I don't want to blow this one up unnecessarily. Thanks :-)

sjsf · 2018-03-22T15:07:50Z

...clipse.smarthome.config.core/src/main/java/org/eclipse/smarthome/config/core/ConfigUtil.java

+     * @return The configuration holder object. All fields that matched a configuration option are set. If a required
+     *         field is not set, null is returned.
+     */
+    public static <T> @Nullable T as(Map<String, Object> properties, Class<T> configurationClass) {


Do you have any particular reasons why this method must/should be exposed as API? I'd rather have expected to see in in a class within an internal package. In that way the Configuration class can use it and everybody else can do a new Configuration(Map).as(Class) if needed. What did I miss?

@SJKA I tried to explain this in the PR message :)
new Configuration(Map).as(Class) does more than we need for service configuration objects. It "normalizes" values, which basically means all numbers -> BigDecimal, Collections -> Lists.

But the ConfigUtils.as method is way smarter, because it converts the types based on the field type. So yes, it makes sense to have it public API and use ConfigUtils.as within services.

If #5300 gets realized, the as() method might move to another package though. Maybe it should be marked as deprecated so that nobody starts to use it before this is discussed.

I tried to explain this in the PR message

If I didn't get that from your explanation, how should a user of the API figure that out? That is pretty surprising.

Another thing worth mentioning is that static utility methods only got us into trouble, both on testing/mocking side as well as with respect to evolving the APIs. We have been working in the past on getting rid of several such static helper methods and that always was painful on some end. Therefore I'd like to object to adding yet another one.

Maybe it should be marked as deprecated so that nobody starts to use it before this is discussed.

I don't know which "as" method you mean:

Configuration.as(...) must stay where it is, that cannot be moved anymore.

ConfigUtil.as.(...) must not be introduced publicly if we already suspect it will have to move. Why does it have to be public then if nobody should start using it?

davidgraeff · 2018-03-26T15:18:40Z

It's hard to mock static methods true.

And yes documentation of this class is not very rich. The pr addresses some methos.

Please tell me how to proceed here. The mqtt stuff is once again ready and API changes of this pr are a requirement for my other PRs. The "as" method does not need to be moved in this PR, but it leads to inefficient code if it is not addressed. (Double type conversion of config values)

htreu · 2018-03-27T13:21:32Z

@davidgraeff, @SJKA to get this one finally approved I suggest to provide the APIs just as they are available now. Please refactor the public static as() method into some internal package. This gives us room for discussion and the rest of MQTT can finally move on.

* getAllFields, as methods moved from Configuration to internal ConfigMapper * Add String->BigDecimal conversion in ConfigMapper.as * Add configuration holder object to MagicService of the magic binding to see if the mapping works for all kind of configuration values. * Sort ConfigUtils: public first. Signed-off-by: David Graeff <david.graeff@web.de>

davidgraeff · 2018-03-27T13:53:20Z

@htreu, @SJKA Done

htreu

Looks good to me, thanks!

htreu · 2018-03-28T05:53:31Z

@davidgraeff Travis fails due to missing author-tag & JavaDoc on the new ConfigMapper. Please add this asap. Thanks :-)

htreu · 2018-03-28T06:18:43Z

Never mind, fixed it with #5320.

davidgraeff force-pushed the configutils branch 3 times, most recently from 17ebc58 to e93ffe0 Compare March 21, 2018 16:04

htreu reviewed Mar 21, 2018

View reviewed changes

davidgraeff force-pushed the configutils branch from e93ffe0 to 15ad511 Compare March 22, 2018 10:49

htreu suggested changes Mar 22, 2018

View reviewed changes

davidgraeff mentioned this pull request Mar 22, 2018

OSGi services: Generate config xmls through annotations #5300

Open

davidgraeff force-pushed the configutils branch from 15ad511 to 3d00326 Compare March 22, 2018 13:13

davidgraeff force-pushed the configutils branch from 3d00326 to f9f7ec5 Compare March 22, 2018 13:47

triller-telekom mentioned this pull request Mar 22, 2018

Security API #4267

Closed

sjsf reviewed Mar 22, 2018

View reviewed changes

davidgraeff force-pushed the configutils branch from f9f7ec5 to 7859b60 Compare March 27, 2018 13:51

htreu approved these changes Mar 27, 2018

View reviewed changes

htreu merged commit 9bf79ed into eclipse-archived:master Mar 27, 2018

htreu mentioned this pull request Mar 28, 2018

ConfigMapper: Add author tag & JavaDoc #5320

Merged

kaikreuzer mentioned this pull request Apr 23, 2018

core.config.ConfigurationService IllegalArgumentException with Magic binding #5291

Closed

davidgraeff deleted the configutils branch July 9, 2018 10:38

htreu added this to the 0.10.0 milestone Oct 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configuration: Move static methods to ConfigUtil. Add documentation. #5292

Configuration: Move static methods to ConfigUtil. Add documentation. #5292

davidgraeff commented Mar 21, 2018 •

edited

Loading

htreu left a comment

htreu Mar 21, 2018

davidgraeff Mar 21, 2018 •

edited

Loading

htreu commented Mar 21, 2018

davidgraeff commented Mar 21, 2018

davidgraeff commented Mar 22, 2018

davidgraeff commented Mar 22, 2018

htreu left a comment

htreu Mar 22, 2018

htreu Mar 22, 2018

davidgraeff Mar 22, 2018 •

edited

Loading

htreu Mar 22, 2018

htreu Mar 22, 2018

davidgraeff Mar 22, 2018

htreu Mar 22, 2018

davidgraeff commented Mar 22, 2018

davidgraeff commented Mar 22, 2018 •

edited

Loading

htreu commented Mar 22, 2018

davidgraeff commented Mar 22, 2018

htreu commented Mar 22, 2018

sjsf Mar 22, 2018

davidgraeff Mar 22, 2018 •

edited

Loading

sjsf Mar 26, 2018

davidgraeff commented Mar 26, 2018

htreu commented Mar 27, 2018

davidgraeff commented Mar 27, 2018

htreu left a comment

htreu commented Mar 28, 2018

htreu commented Mar 28, 2018

Configuration: Move static methods to ConfigUtil. Add documentation. #5292

Configuration: Move static methods to ConfigUtil. Add documentation. #5292

Conversation

davidgraeff commented Mar 21, 2018 • edited Loading

htreu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidgraeff Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

htreu commented Mar 21, 2018

davidgraeff commented Mar 21, 2018

davidgraeff commented Mar 22, 2018

davidgraeff commented Mar 22, 2018

htreu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidgraeff Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidgraeff commented Mar 22, 2018

davidgraeff commented Mar 22, 2018 • edited Loading

htreu commented Mar 22, 2018

davidgraeff commented Mar 22, 2018

htreu commented Mar 22, 2018

Choose a reason for hiding this comment

davidgraeff Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidgraeff commented Mar 26, 2018

htreu commented Mar 27, 2018

davidgraeff commented Mar 27, 2018

htreu left a comment

Choose a reason for hiding this comment

htreu commented Mar 28, 2018

htreu commented Mar 28, 2018

davidgraeff commented Mar 21, 2018 •

edited

Loading

davidgraeff Mar 21, 2018 •

edited

Loading

davidgraeff Mar 22, 2018 •

edited

Loading

davidgraeff commented Mar 22, 2018 •

edited

Loading

davidgraeff Mar 22, 2018 •

edited

Loading