WFLY-19304: update model definition and bump schema #17866
Conversation
|
@tadamski could I get a review? I'm planning to update the rest of the connector subsystems too. |
|
@bstansberry could I get someone to look this over? |
There was a problem hiding this comment.
@michpetrov sorry for late review. The PR looks good to me regarding configuration processing. I'm just wondering: if the changes from 7.1 to 7.2 are a bugfix (ammending wrong configuration instead of adding anything new), maybe this should be treated as 7.1 fix rather than schema update. OTOH, if the new schema is needed - don't we need any 7.2 -> 7.1 transformers?
|
@tadamski I assumed making changes to schema requires a version bump no matter what but if not then this can be a fix for 7.1, I don't think there's anything that requires a transformer, the order of the elements changes but the parser doesn't care about that. |
|
ok, so iiuc this fix makes sure that configuration that was parsed and persisted is consistent with the schema, but it should have no impact on any correct configuration neither in 7.1 nor 7.2 schema. tbh I'm still not certain if in such case a version bump is needed, and another question is: I assume all previous configs would have this problem as well so should we treat it like a bug and ammend all previous xsd that are still supported, or indeed just create 7.2 and say that this is a version since which the config bug was fixed. @bstansberry wdyt? |
| @@ -431,7 +433,8 @@ public class Constants { | |||
| static SimpleAttributeDefinition TRACK_STATEMENTS = new SimpleAttributeDefinitionBuilder(TRACKSTATEMENTS_NAME, ModelType.STRING, true) | |||
| .setAllowExpression(true) | |||
| .setXmlName(Statement.Tag.TRACK_STATEMENTS.getLocalName()) | |||
| .setDefaultValue(new ModelNode(Defaults.TRACK_STATEMENTS.name())) | |||
| .setDefaultValue(new ModelNode(Defaults.TRACK_STATEMENTS.toString())) | |||
| .setAllowedValues(Arrays.stream(Statement.TrackStatementsEnum.values()).map(Statement.TrackStatementsEnum::toString).toArray(String[]::new)) | |||
There was a problem hiding this comment.
This is a breaking change as it forces the user to provide the lower case value, while in the past AFAICT the code would allow either upper or lower case.
Use EnumValidator. It is agnostic about case and is also less verbose.
| @@ -146,7 +148,7 @@ public class DataSourcesExtension implements Extension { | |||
| public static final String SUBSYSTEM_NAME = Constants.DATASOURCES; | |||
| private static final String RESOURCE_NAME = DataSourcesExtension.class.getPackage().getName() + ".LocalDescriptions"; | |||
|
|
|||
| static final ModelVersion CURRENT_MODEL_VERSION = ModelVersion.create(6, 0, 0); | |||
| static final ModelVersion CURRENT_MODEL_VERSION = ModelVersion.create(7, 2, 0); | |||
There was a problem hiding this comment.
Note: this being at 6, 0, 0 instead of 7,0,0 was a serious bug.
There was a problem hiding this comment.
Well, a potentially serious bug. It seems 7.0.0 and 6.0.0 were equivalent in terms of domain mode, so the bug did no harm.
| @@ -492,6 +497,9 @@ public class Constants { | |||
| static SimpleAttributeDefinition TRANSACTION_ISOLATION = new SimpleAttributeDefinitionBuilder(TRANSACTION_ISOLATION_NAME, ModelType.STRING, true) | |||
| .setXmlName(DataSource.Tag.TRANSACTION_ISOLATION.getLocalName()) | |||
| .setAllowExpression(true) | |||
| .setAllowedValues(TransactionIsolation.TRANSACTION_NONE.name(), TransactionIsolation.TRANSACTION_READ_COMMITTED.name(), | |||
| TransactionIsolation.TRANSACTION_READ_UNCOMMITTED.name(), TransactionIsolation.TRANSACTION_REPEATABLE_READ.name(), | |||
| TransactionIsolation.TRANSACTION_SERIALIZABLE.name()) | |||
There was a problem hiding this comment.
This validation is incorrect per the existing code. See the uses of TransactionIsolation.customLevel(...) which allows arbitrary values.
Changing that would be a breaking change and the API would still have to allow the values on an HC for mixed-domain use, while rejecting them on a currently running server. Such a change should be handled under a different JIRA, assuming it's wanted.
|
@tadamski re your policy questions, AIUI there are three basic changes here:
I believe that these changes have no functional impact vs the existing behavior (or won't once my comments about the validators are addressed) so in a crisis we might skip the management API version bump. But given the magnitude of the related changes and the presence of the important-to-the-user validation information, we should bump the management API. I don't see a need for new transformer code, because there is no change in what a DC processing this 7.2 version of the api would send to an HC that uses an earlier version.
The existing handling of transaction-isolation appears to be inconsistent with the xsd, since arbitrary values are allowed (see TranactionIsolation.customLevel) while the xsd says they are not allowed. But that doesn't require us to update existing xsds -- parsers can be more lenient than the xsd. Really the new xsd shouldn't have the restriction around these; having it would only be because TranactionIsolation.customLevel is a kind of hidden feature, e.g. kept for backward compatibility but not something we want to advertise via the xsd.
WF requires subsystems to marshal in a format consistent with the xsd, and that includes the order of elements and attributes. The ordering requirement is for good UX -- if a user provides a document that is ordered per the current xsd, we should not reorder when we marshal. We can't marshal in the same order they used if it's inconsistent with the xsd, as we don't preserve the ordering info in memory, but at least we can avoid irrelevant document changes if they order per the xsd. So, if we are marshaling in an order inconsistent with the current xsd, we have a bug that must be fixed. The fix is either to fix the marshaling, or if we think the marshaling is the order we want, then we issue a new xsd. It sounds like in this case the fix is doing both. Note that documents used in the subsystem test should be written in xsd order (at least those that use the current xsd), so the subsystem test can validate that the marshaling is also in xsd order, by comparing the input and output xml. I assume that wasn't case previously. |
Issue: WFLY-19304
Updated the schema to reflect what's actually produced by the XmlWriter. The bigger part is keeping the elements in the correct order, though I did change the order in the writer as well to keep it the same between
datasourceandxa-datasource. Since the parser doesn't care about the order this should not affect backwards compatibility.Second part is changing some elements from "boolean presence" -
<element />to simple boolean -<element>true</element>. The elements declared as "boolean presence" were neither written nor parsed as such,<elem />and<elem>true</elem>should be equivalent but<elem />is actually considered "false", this is potentially breaking backwards compatibility but if so it was broken already.Thirdly two attributes had allowed values added since that's what's expected.
More information about the wildfly-bot[bot]