Empty values option 2 #445

dmlloyd · 2019-09-06T21:50:50Z

This is the second option for an API to support the new consistent rules for empty value handling, by creating a fluent accessor API instead of using method overloads. Supercedes #407. Fixes #446.

eclipse-microprofile-bot · 2019-09-06T21:53:01Z

Can one of the admins verify this patch?

eclipse-microprofile-bot · 2019-09-06T21:53:02Z

Can one of the admins verify this patch?

dmlloyd · 2019-09-06T22:10:38Z

Before moving the PR out of draft:

Choose this approach and close Empty values option 1 #444
Ensure that a conforming implementation is possible; revise as needed
Complete and test TCK additions necessary to test conformance
Rebase after Revert to 1.3 + cherry-picks #443 or equivalent is merged
Add Signed-off-by

Emily-Jiang · 2019-09-06T22:25:13Z

@dmlloyd You need to sort your ECA. The other thing - can you associate this PR with an issue. It might be better to open an issue to clearly specify the problem. It might make sense to copy some statement from PRs. When we release, we list issues. The closed unmerged PRs will be discarded.

dmlloyd · 2019-09-09T13:28:37Z

ECA is sorted. I'll open an issue.

njr-11 · 2019-09-09T16:18:40Z

spec/src/main/asciidoc/converters.asciidoc

+| `" "` | `String` | `getOptionalValue` | `Optional.of(" ")`
+| `"foo"` | `String` | `getOptionalValue` | `Optional.of("foo")`
+| missing | `String[]` | `getOptionalValue` | `Optional.empty()`
+| `""` | `String[]` | `getOptionalValue` | `Optional.empty()`


Some comment applies here that I added under #444,
https://github.com/eclipse/microprofile-config/pull/444/files/#r322320335

Emily-Jiang · 2019-09-10T22:53:51Z

This approach is closer to what we have in master. With this, I think it might make better sense just deleting the snapshot and then reserve the copyright. It is not right to use the original idea but remove the original contribution. All contributors are listed under ConfigAccessor needs to be added. I think it is cleaner to delete unwanted methods and then rework. In this case, we can move faster and there won't be any copyright complication. Thoughts?

Emily-Jiang · 2019-09-10T22:54:57Z

@dmlloyd You need to use -s to sign off. "David Lloyd (david.lloyd@****.com) did not include the "Signed-off-by footer" which is required for all commits made by a contributor."

dmlloyd · 2019-09-11T02:36:49Z

The original accessor was builder-based and doesn't share a lot of similarities with the original in usage AFAICT. But I'm fine with adding contribution info, if the original author(s) of the original accessor approach want it.

I'll add the sign-off once the checklist above is complete.

dmlloyd · 2019-09-11T13:32:29Z

I have a plausible implementation of the accessor API here: https://github.com/dmlloyd/smallrye-config/tree/accessor so that should be sufficient to demonstrate the workability of the API.

dmlloyd · 2019-09-11T13:33:43Z

Also I don't like how the spec language is a bit inconsistent so I'll probably add commits to clean that up, either as part of this or as a separate PR. But I've filed #447 in any event.

dmlloyd · 2019-09-16T19:56:59Z

I've updated this version (locally) to rename getAccessor to access (similarly to the original/existing version). But I won't push it up until #443 is resolved one way or another because I will need to rebase the whole thing (either way).

Emily-Jiang

I suggest to use the original builder pattern. @struberg please comment.

Emily-Jiang · 2019-09-25T10:29:51Z

api/src/main/java/org/eclipse/microprofile/config/ConfigAccessor.java

+ * @throws IllegalStateException if the accessor is optional, or if the accessor is already associated with a
+ * type-specific default value, or if there is no converter available for the given type
+ */
+ <U> ConfigAccessor<U> convertedForType(Class<U> clazz) throws IllegalStateException;


I think this pattern leads to the creation of ConfigAccessor being mutable or thrown away ConfigAccessor. This is why we introduced the builder pattern in the first place.

I would expect a properly tuned JDK to elide the vast majority of these allocations or else detect that they do not escape. The config accessor is not mutable (by specification) so I'm not sure what you mean by saying it's mutable.

The "fluent" style of API is widely used already. Using a builder on the other hand is much more unwieldy from a user's perspective, for something that will be thrown away immediately.

Here are some other things to consider:

Using the fluent immutable API means the result is perfectly type safe. The builder approach means that no mutations are allowed which would change the type of the result (otherwise ugly tricks like unsafely re-typing the object would have to be employed). If (with the builder approach) you forbid re-typing, the type or converter must be passed in to access and cannot be changed later; this also means the accessor result must have separate methods for every result type (for example, optional values would need their own method, as would collection values (if those were ever added), etc.).

Using the builder API, you end up with an extra, strange build step. These two things together make the API more clunky and decrease its flexibility: config.access("some.prop", Foo.class).build().getOptionalValue() versus config.access("some.prop").withType(Foo.class).optional().getValue(). It's not entirely obvious that to the user that calling build is necessary, whereas with the immutable option, IDE completion would make it clear what operations are possible in a fairly unambiguous way based on the result type and possible method completions.

The fluent immutable API also reflects the immutability of the config object itself (not reflecting the mutability of the backing data, but the actual configuration object and its configuration from an API perspective).

I agree with David, this pattern is fine and make things type safe and consistent.

cs = config.access("some.prop").withType(Foo.class).WithDefault("blah") cs.getValue() -> "blah" cs.WithDefault("Yes!") // <- this does not mutate the cs instance cs.getValue() -> "blah" // If users want to provide another default, he has to take the returned ConfigAccessor: cs.withDefault("Yes!").getValue() -> "Yes!"

In most cases, users won't even keep a reference on the ConfigAccessor and will directly take the returned value for getValue().

It's very similar to the original design and mixes up builder and ConfigAccessor. It's basically the old ConfigValue. We have been there over a year ago.

I prefer the builder pattern, which was in the master before. With the build pattern, the usage will be a functional style. You build one configAccessor and use it.

* [Spec] add section explaining how the Config API handles a config property with missing or empty value * [Spec] mention that it is possible to set an empty value to reset a property from a lower-ordinal config source. * [TCK] add EmptyConfigPropertyTest to test all cases of empty config property Co-authored-by: David M. Lloyd <david.lloyd@redhat.com>

Signed-off-by: David M. Lloyd <david.lloyd@redhat.com>

struberg · 2019-10-01T12:22:07Z

@dmlloyd This is very, very similar approach to what was originally proposed over 3 years ago.
Please compare with https://github.com/struberg/javaConfig/blob/configValue/api/src/main/java/javx/config/ConfigValue.java

The design is derived from the work we've done in Apache DeltaSpike in 2011
https://github.com/apache/deltaspike/blob/master/deltaspike/core/api/src/main/java/org/apache/deltaspike/core/api/config/ConfigResolver.java#L611

Emily-Jiang

I think the better approach is to use builder pattern. In this way, it is nice and clean. Just get hold of a builder and build an accessor. When building it, you can use different default or Converter.

Emily-Jiang · 2019-10-01T12:48:16Z

@struberg The current code was influenced by the previous ConfigAccessor code, which was derived from the ConfigJSR conversation (DeltaSpike influcences).

dmlloyd · 2019-10-01T13:14:03Z

@dmlloyd This is very, very similar approach to what was originally proposed over 3 years ago.

So I assume this means you don't have any objections to this approach.

Please compare with https://github.com/struberg/javaConfig/blob/configValue/api/src/main/java/javx/config/ConfigValue.java

One difference I see here is that it uses unsafe casting to use the same mutable instance, as opposed to returning a new typesafe instance.

The design is derived from the work we've done in Apache DeltaSpike in 2011
https://github.com/apache/deltaspike/blob/master/deltaspike/core/api/src/main/java/org/apache/deltaspike/core/api/config/ConfigResolver.java#L611

Yes, this is very similar to my proposed approach.

dmlloyd · 2020-03-05T17:20:26Z

Plan is to reopen this as new issues under the new organizational scheme.

dmlloyd mentioned this pull request Sep 6, 2019

Revert to 1.3 + cherry-picks #443

Merged

dmlloyd force-pushed the empty-values-option-2 branch from 6184efc to a36f495 Compare September 6, 2019 22:08

dmlloyd mentioned this pull request Sep 6, 2019

Empty values option 1 #444

Closed

This was referenced Sep 9, 2019

Consistent empty/missing behavior #446

Closed

Specify handling of empty config property #407

Closed

njr-11 reviewed Sep 9, 2019

View reviewed changes

dmlloyd force-pushed the empty-values-option-2 branch from a36f495 to 74a8c9f Compare September 9, 2019 19:41

Emily-Jiang requested changes Sep 25, 2019

View reviewed changes

dmlloyd mentioned this pull request Sep 26, 2019

Must a Config actually use Converters? #448

Closed

dmlloyd and others added 4 commits September 27, 2019 13:03

Empty and missing values: Specification update

1ff140c

Add getConverter() method to Config

9a69ed9

Config accessor to support empty values

944c951

Signed-off-by: David M. Lloyd <david.lloyd@redhat.com>

dmlloyd force-pushed the empty-values-option-2 branch from 74a8c9f to 944c951 Compare September 27, 2019 18:17

Emily-Jiang requested changes Oct 1, 2019

View reviewed changes

dmlloyd mentioned this pull request Oct 2, 2019

Support transformation of configuration values #451

Open

dmlloyd mentioned this pull request Jan 20, 2020

add convenience methods for getting multi-valued properties #496

Closed

dmlloyd closed this Mar 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Empty values option 2 #445

Empty values option 2 #445

dmlloyd commented Sep 6, 2019 •

edited

Loading

eclipse-microprofile-bot commented Sep 6, 2019

eclipse-microprofile-bot commented Sep 6, 2019

dmlloyd commented Sep 6, 2019 •

edited by struberg

Loading

Emily-Jiang commented Sep 6, 2019

dmlloyd commented Sep 9, 2019

njr-11 Sep 9, 2019

Emily-Jiang commented Sep 10, 2019

Emily-Jiang commented Sep 10, 2019

dmlloyd commented Sep 11, 2019

dmlloyd commented Sep 11, 2019

dmlloyd commented Sep 11, 2019

dmlloyd commented Sep 16, 2019

Emily-Jiang left a comment

Emily-Jiang Sep 25, 2019

dmlloyd Sep 25, 2019

dmlloyd Sep 25, 2019 •

edited

Loading

jmesnil Sep 27, 2019

struberg Oct 1, 2019

Emily-Jiang Oct 1, 2019

struberg commented Oct 1, 2019

Emily-Jiang left a comment

Emily-Jiang commented Oct 1, 2019

dmlloyd commented Oct 1, 2019

dmlloyd commented Mar 5, 2020

Empty values option 2 #445

Empty values option 2 #445

Conversation

dmlloyd commented Sep 6, 2019 • edited Loading

eclipse-microprofile-bot commented Sep 6, 2019

eclipse-microprofile-bot commented Sep 6, 2019

dmlloyd commented Sep 6, 2019 • edited by struberg Loading

Emily-Jiang commented Sep 6, 2019

dmlloyd commented Sep 9, 2019

njr-11 Sep 9, 2019

Choose a reason for hiding this comment

Emily-Jiang commented Sep 10, 2019

Emily-Jiang commented Sep 10, 2019

dmlloyd commented Sep 11, 2019

dmlloyd commented Sep 11, 2019

dmlloyd commented Sep 11, 2019

dmlloyd commented Sep 16, 2019

Emily-Jiang left a comment

Choose a reason for hiding this comment

Emily-Jiang Sep 25, 2019

Choose a reason for hiding this comment

dmlloyd Sep 25, 2019

Choose a reason for hiding this comment

dmlloyd Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

jmesnil Sep 27, 2019

Choose a reason for hiding this comment

struberg Oct 1, 2019

Choose a reason for hiding this comment

Emily-Jiang Oct 1, 2019

Choose a reason for hiding this comment

struberg commented Oct 1, 2019

Emily-Jiang left a comment

Choose a reason for hiding this comment

Emily-Jiang commented Oct 1, 2019

dmlloyd commented Oct 1, 2019

dmlloyd commented Mar 5, 2020

dmlloyd commented Sep 6, 2019 •

edited

Loading

dmlloyd commented Sep 6, 2019 •

edited by struberg

Loading

dmlloyd Sep 25, 2019 •

edited

Loading