[#70] Implement a helper Map implementation for ConfigSources to use #72
Conversation
|
Added tests. |
implementation/pom.xml
Outdated
| @@ -69,6 +69,50 @@ | |||
| <forkMode>once</forkMode> | |||
| </configuration> | |||
| </plugin> | |||
| <plugin> | |||
There was a problem hiding this comment.
Should something like this be moved into smallrye-parent so all projects could use it?
There was a problem hiding this comment.
I'd be okay with that I guess.
There was a problem hiding this comment.
But that means there'd have to be a release and so forth.
There was a problem hiding this comment.
True, but pulling the trigger is pretty easy.
Other than needing a release of parent, any reason not to?
There was a problem hiding this comment.
No, sounds fine. Is that something you can do?
There was a problem hiding this comment.
I like this feature and it is genuinely useful.
However, I have some concern with the mutability of the underlying Map leaking to the config source.
@dmlloyd Do your use cases require to modify the properties collections or would it be enough to provide an Unmodifiable Map implementation?
| * The implementation attempts to make no assumptions about the efficiency of the backing implementation and | ||
| * prefers the most direct access possible. | ||
| * <p> | ||
| * Some mutate operations are supported (specifically removals), where they can be performed on the backing key set, if |
There was a problem hiding this comment.
that's not specified in the spec (although it should be) but my interpretation of getPropertyNames() is that it should return an immutable set of names.
In general, ConfigSource should not be used to modify/update configuration. If some of these methods provide ambiguity about it, it should be properly specified in the spec or in the API.
I'll open issues in microprofile-config to clarify these use cases
There was a problem hiding this comment.
There was a problem hiding this comment.
there is also eclipse/microprofile-config#333 where it is discussed
| public String remove(final Object key) { | ||
| final String value = get(key); | ||
| //noinspection SuspiciousMethodCalls - it's OK in this case | ||
| return delegate.getPropertyNames().remove(key) ? value : null; |
There was a problem hiding this comment.
remove could throw a UnsupportedOperationException if the list is unmodifiable
|
The idea was to "punt" on mutability. If the underlying config source is mutable then the map is; if it isn't, then the map isn't. This way it doesn't impose any policy one way or the other. I can make it assume full immutability though if you want me to. |
|
@dmlloyd I would prefer full immutability (the goal being then to propose this improvement to microprofile-config to update its API wrt I don't like that the application could change the config sources. Having mutability raise expectation that the application knows the config source that always provides the property which is the opposite of the design behind the The whole microprofile-config API is really only about viewing the configuration. The update/modification of the configuraiton is out of scope of the specification. |
|
OK I'll push an immutable version. |
dmlloyd
force-pushed
the
map
branch
2 times, most recently
from
284d707 to
ab2a7f9
Compare
|
OK pushed (also remembered to update the tests ;) ) |
|
I think CI might have been confused by my rapid pushes. Can we trigger a retest? |
|
I guess I'll just re-amend and push a new commit ID |
|
Hmm |
|
Ah it's the JavaDoc issue. We need Ken's parent update. |
|
v2 has been pushed to Sonatype, basically waiting for percolation to Central. |
|
|
|
Thanks Ken, I'll update the PR. |
|
@dmlloyd could you please rebase and I'll merge it? |
|
Done. |
No description provided.