Allow empty configuration values for String injection points #2766
Conversation
...camel-servlet/runtime/src/main/java/io/quarkus/camel/servlet/runtime/CamelServletConfig.java
Outdated
Show resolved
Hide resolved
| @@ -38,6 +38,9 @@ | |||
| @Inject | |||
| Config config; | |||
|
|
|||
| @ConfigProperty(name = "greeting.empty") | |||
| String empty; | |||
|
|
|||
There was a problem hiding this comment.
would be good to also test the Optional case to be sure it gets an Optional<String> with an empty String
There was a problem hiding this comment.
actually, this is not correct according to the comment made above (getOptionalValue returns an empty Optional when the string value is an empty string this should not be considered an error).
I guess an Optional with an empty string must be injected in this case, not an empty optional.
There was a problem hiding this comment.
Actually we don't really control that part, the value comes from MP config (we just create the config source), so I'm not really sure what we should do.
Any ideas?
There was a problem hiding this comment.
@cescoffier the TCK passes, so I think this is safe. WDYT?
|
Would be curious to know if this changes with updating SmallRye Config. I vaguely recall some changes around empty strings but not certain. Do you recall @dmlloyd? |
|
There is currently a disagreement in the MP Config community about handling of empty values. The discussion is primarily here: eclipse/microprofile-config#407 If you agree with my proposal, then the correct behavior is to treat empty and missing as identical. In this case, if the intent of injection plain fields is to fail on a missing value, then it also would mean failing on an empty value. |
|
OK, so I guess for the moment we shouldn't change anything here until we have a resolution |
|
Description:The MP Config hangout is at 6am EDT on Fridays:
dial in details: https://eclipse.zoom.us/j/949859967 <https://www.google.com/url?q=http%3A%2F%2Feclipse.zoom.us%2Fj%2F949859967&sa=D&ust=1560931748753000&usg=AFQjCNHZXDMFF3DtVu3zPgMZA8bIo1x-NQ>
Minutes: https://docs.google.com/document/d/1X6Q9K28VNHhVRkVBFlbNa-ZMusxjCfmoA7--uJFHWx0 <https://docs.google.com/document/d/1X6Q9K28VNHhVRkVBFlbNa-ZMusxjCfmoA7--uJFHWx0>
There has not been one for a while, so perhaps you can try to get it resolved on the 21st by asking for a meeting. I did start a google group thread asking to get this teed up for next week or to finalize the discussion on the PR.
https://groups.google.com/forum/#!topic/microprofile/L7n3JZRHtm8 <https://groups.google.com/forum/#!topic/microprofile/L7n3JZRHtm8>
… On Jun 13, 2019, at 5:47 AM, David M. Lloyd ***@***.***> wrote:
There is currently a disagreement in the MP Config community about handling of empty values. The discussion is primarily here: eclipse/microprofile-config#407 <eclipse/microprofile-config#407>
If you agree with my proposal, then the correct behavior is to treat empty and missing as identical. In this case, if the intent of injection plain fields is to fail on a missing value, then it also would mean failing on an empty value.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <#2766?email_source=notifications&email_token=AACRDMUHPOJCNDTYCFMYG33P2I6VTA5CNFSM4HWJCU6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXTSLII#issuecomment-501687713>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACRDMQJQ2QWQSF6BRYBR6TP2I6VTANCNFSM4HWJCU6A>.
|
|
Emily did say on the google group that this was discussed without resolution last week, and they will discuss it again today. I’m not going to make it as it’s 3am my time and I’m heading to bed now just before 2am.
… On Jun 14, 2019, at 1:23 AM, Scott Stark ***@***.***> wrote:
Description:The MP Config hangout is at 6am EDT on Fridays:
dial in details: https://eclipse.zoom.us/j/949859967 <https://www.google.com/url?q=http%3A%2F%2Feclipse.zoom.us%2Fj%2F949859967&sa=D&ust=1560931748753000&usg=AFQjCNHZXDMFF3DtVu3zPgMZA8bIo1x-NQ>
Minutes: https://docs.google.com/document/d/1X6Q9K28VNHhVRkVBFlbNa-ZMusxjCfmoA7--uJFHWx0 <https://docs.google.com/document/d/1X6Q9K28VNHhVRkVBFlbNa-ZMusxjCfmoA7--uJFHWx0>
There has not been one for a while, so perhaps you can try to get it resolved on the 21st by asking for a meeting. I did start a google group thread asking to get this teed up for next week or to finalize the discussion on the PR.
https://groups.google.com/forum/#!topic/microprofile/L7n3JZRHtm8 <https://groups.google.com/forum/#!topic/microprofile/L7n3JZRHtm8>
> On Jun 13, 2019, at 5:47 AM, David M. Lloyd ***@***.*** ***@***.***>> wrote:
>
> There is currently a disagreement in the MP Config community about handling of empty values. The discussion is primarily here: eclipse/microprofile-config#407 <eclipse/microprofile-config#407>
> If you agree with my proposal, then the correct behavior is to treat empty and missing as identical. In this case, if the intent of injection plain fields is to fail on a missing value, then it also would mean failing on an empty value.
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub <#2766?email_source=notifications&email_token=AACRDMUHPOJCNDTYCFMYG33P2I6VTA5CNFSM4HWJCU6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXTSLII#issuecomment-501687713>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACRDMQJQ2QWQSF6BRYBR6TP2I6VTANCNFSM4HWJCU6A>.
>
|
|
Unfortunately I woke up too late to make the call, which I guess must have been at 5am my time. The conclusion is far from satisfactory though: it actually adds more confusing behavior. |
|
What is the best way to try to drive this to the conclusion we want? Does the updated tck/src/main/java/org/eclipse/microprofile/config/tck/EmptyValuesTest.java enumerate every possible variation of the empty value scenario so there is one place to see the impact of how this is handled? |
|
@kenfinnigan should we ask our guys from smallrye-config to participate in this discussion? |
|
@kenfinnigan who's winning? |
|
MP Config I think at present |
|
The crows |
|
OK, I pitched in. Couldn't even make it worse given how stuck this is. |
|
We've been slowly making progress on discussions around this issue on the spec calls, but it's still unresolved. |
|
Thanks for the update @dmlloyd ! |
do you have an update @dmlloyd ? |
|
The existing behavior stands. If an empty value is allowed, the property must be of type If we mapped empty values to empty string, then we would have no way to declare that a |
Fixes #2765