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

Should config_ordinal leak into Config? #676

Open
radcortez opened this issue Feb 17, 2021 · 14 comments
Open

Should config_ordinal leak into Config? #676

radcortez opened this issue Feb 17, 2021 · 14 comments
Milestone

Comments

@radcortez
Copy link
Contributor

Description

The config_ordinal is just a local property to a ConfigSource to help with the sort. Does it make sense for config_ordinal to be available in Config.getPropertyNames? Or to return a value in Config.getValue (and derivatives)?

In my opinion I don't think so. As mentioned, config_ordinal is associated with a single ConfigSource with no relationship with the others. Making a call to Config.getValue does not have any meaning. It is just the highest available ordinal and we don't even know the ConfigSource. Same for other methods, like Config.getPropertyNames or Config.getValues.

@Emily-Jiang
Copy link
Member

Emily-Jiang commented Feb 17, 2021

The property config_ordinal is special and it is used to indicate the ordinal value of the associated config source as you also noted. It should not be surfaced outside the config source. Besides its value can be retrieved via configsource.getOrdinal().

@radcortez
Copy link
Contributor Author

Ok, then we probably need to add some TCK tests for this.

@ljnelson
Copy link
Contributor

I'd like to understand what is being proposed.

Currently Config.getPropertyNames() can return whatever it wants (and still be specification compliant), yes?

Is there a proposal in this issue to make config_ordinal a "magic" property name, such that someConfigSource.getValue("config_ordinal") should always return null, for all ConfigSource implementations?

@radcortez
Copy link
Contributor Author

Currently Config.getPropertyNames() can return whatever it wants (and still be specification compliant), yes?

Yes. I guess we could leave it as is, but I was mostly wondering if there is any worth is returning it?

Is there a proposal in this issue to make config_ordinal a "magic" property name, such that someConfigSource.getValue("config_ordinal") should always return null, for all ConfigSource implementations?

I think that ConfigSource.getValue("config_ordinal") should return a value, but Config.getValue("config_ordinal") should not return one. In practice, config_ordinal is a magic property.

@ljnelson
Copy link
Contributor

In practice, then, doesn't this mean that a Config#getPropertyNames() implementation that wants to do the right thing needs to filter the individual return values of ConfigSource#getPropertyNames() to exclude "config_ordinal" from the sequence that the caller will iterate over?

@radcortez
Copy link
Contributor Author

Yes.

@ljnelson
Copy link
Contributor

ljnelson commented Feb 18, 2021

So it is a property, because a ConfigSource can return a value for it (I went looking to see if I could find text that indicates that a Config must expose all and only the property values that its ConfigSources expose, but I couldn't find it, but I believe that is the intention of the overall MicroProfile Config effort, i.e. a Config does not add state above and beyond that surfaced by its ConfigSources). But it is not a property in that a Config#getValue() call should not surface it (so with this proposal a Config now quietly changes its nature and becomes a state-subtracting entity). This is all extraordinarily weird IMHO.

@ljnelson
Copy link
Contributor

ljnelson commented Feb 18, 2021

Perhaps the larger issue goes something like this:

  • There are some things about configuration that concern the configuration itself. Let's call this metaconfiguration.
    • (Many people have many opinions on how to express metaconfiguration. I don't want to go into those opinions here.)
  • One bit of metaconfiguration that exists somewhat hazily in this specification is the config_ordinal property. It is a magic property in some ill-defined sense that is used for sorting purposes. Another might be the fact that a ConfigSource's name, apparently, maybe, is used as a tiebreaker in sorting, but no guidance is given nor restrictions placed on what that name must or should be.
  • There may come to be other such pseudo-magic metaconfiguration down the road.
  • How should the specification express such metaconfiguration in a general, rigorous manner? Currently it has taken a very ad hoc approach. For example, we have this issue, and we also have the long-drawn-out discussion about how to indicate the removal of a property value versus its emptiness (which along its journey has confused metaconfiguration with configuration). We also have the issue of default configuration values, with wide-ranging opinions, which one could argue treads on metaconfiguration territory.

@ljnelson
Copy link
Contributor

ljnelson commented Feb 18, 2021

So: back to the concrete details of this very particular issue. My take is this: either config_ordinal is a metaconfiguration artifact, and thus not at all part of the property names and property values represented by the overall configuration system, or it is not. Clearly it is. Therefore it must be "invisible" wherever possible. More rigorously:

  • The return value of configSource.getValue("config_ordinal") MUST be null for any configSource at any time.
  • The return value of config.getOptionalValue("config_ordinal", anyType) for any config and any anyType at any time MUST be a non-null, empty Optional (its isPresent() method MUST return false).
  • A Java String equal to "config_ordinal" MUST NOT at any time be yielded by Iterator#next() when iterating over the Iterable returned by Config#getPropertyNames().
  • A Java String equal to "config_ordinal" MUST NOT be contained by the Set of Strings returned by configSource.getPropertyNames(), for any configSource, at any time.
  • NO implementation, including the default, of ConfigSource#getProperties() may return a Map that contains a key equal to a Java String equal to "config_ordinal", at any time.

Now, why is this particular property name special? Does it start with something like . or _ or…? Is there some way to determine that it concerns metaconfiguration? No, it does not, and there is no such way. It is special because it is historically magic and for no other reason. This is a larger issue that needs to be remedied in future versions of this specification.

As meta-commentary: this took me several drafts and five bullet points to spell out the rigor around this one tiny nugget of metaconfiguration. A larger discussion in this area is IMHO clearly needed.

@Emily-Jiang
Copy link
Member

After thinking the issue all over and reading all of the comments, here is my 2nd thought.
The property config_ordinal is associated with the embed configsource to indicate its ordinal. It is a reserved property, as stated in my previous comment.

We could stop this property from surfacing out. However, do we gain anything by doing so? I think there is a valid use case to surface this property:

As an end users, I would like to find out the highest ordinal number among my config sources. If someone calls config.getValue("config_ordinal", Integer.class), the highest config_ordinal will be returned.

Considering the cons/pros with supressing this property (treating this property special) vs. surfacing this property (normal property), I think surfacing this property has less impact to the existing customers. If we suppressing this property, the current config.getValue("config_ordinal", Integer.class) will throw exception.

Since the spec said this config_ordinal is a property, I think we are good. Do we want to add a tck to enforce this or leave it as it is?
Thoughts?

@ljnelson
Copy link
Contributor

The property config_ordinal is associated with the embed configsource to indicate its ordinal.

It is configuration about the configuration system, in other words, and thus metaconfiguration.

It is a reserved property, as stated in my previous comment.

It is "reserved" because it is metaconfiguration but is mixed in with regular configuration. That's why it has these seemingly magical characteristics, not because it is "reserved". (What would be the point of reserving configuration? There could be none.)

I am fine if we decide basically: "Oops, we messed up; config_ordinal never really should have been in there, but it is, so we treat it no differently from any other property".

I am not fine if we decide: "Yes, it makes perfect sense for metaconfiguration and configuration to mix like this without any way of telling which is which".

I started this discussion not really agreeing with @radcortez's position and I am coming to the end basically supporting a stronger version of it.

@radcortez
Copy link
Contributor Author

@ljnelson thanks for the detailed proposal. My main point was also to discuss this matter widely and figure out where should we move.

I'm +1 on it.

@ljnelson
Copy link
Contributor

I'm (obviously) +1 on it too, but I still want to think about it because it is another breaking change. It is a good breaking change, but breaking nonetheless.

Thinking bigger: what would the process be within MicroProfile (I should know this as a committer but I don't) to open discussion about larger issues like {waves hands} metaconfiguration in general? Opening a Github issue seems like a terrible tool for such a thing, and hangouts or meetups seem too transient. I've seen some excellent "larger" Github issues in the area of config simply be closed and effectively ignored and so I don't want that to happen.

@radcortez
Copy link
Contributor Author

I don't believe there is a strict process for it, but probably the mailing list would raise more awareness and bring more people into the discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants