mbeckerle commented on code in PR #1582:
URL: https://github.com/apache/daffodil/pull/1582#discussion_r2481579690
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/AnnotatedSchemaComponent.scala:
##########
@@ -107,10 +107,24 @@ trait ResolvesScopedProperties extends FindPropertyMixin
{ self: Term =>
private def findDefaultProperty(pname: String): PropertyLookupResult = {
val result = findPropertyInSources(pname, defaultPropertySources)
val fixup = result match {
- case Found(value, loc, pname, _) =>
+ case Found(value, loc, pname, _) => {
+ // we found the property in default format annotation. Daffodil 4.1.0
reversed the order
+ // we resolved default property sources to correctly match the DFDL
specification.
+ // Fortunately, this does not affect most schemas, but for those that
it does, it can be
+ // very difficult to determine what the old value was. So we lookup
property sources in
+ // the old incorrect reversed and see if we find the same value and
warn if not
Review Comment:
incorrect reversed _order_ and ...
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/AnnotatedSchemaComponent.scala:
##########
@@ -357,16 +371,33 @@ trait AnnotatedSchemaComponent
}
}.value
- final protected lazy val defaultPropertySources: Seq[ChainPropProvider] =
- LV(Symbol("defaultPropertySources")) {
+ /**
+ * The default property sources we should use for resolving property values
is calculated by
Review Comment:
is => are
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/AnnotatedSchemaComponent.scala:
##########
@@ -357,16 +371,33 @@ trait AnnotatedSchemaComponent
}
}.value
- final protected lazy val defaultPropertySources: Seq[ChainPropProvider] =
- LV(Symbol("defaultPropertySources")) {
+ /**
+ * The default property sources we should use for resolving property values
is calculated by
+ * combining the default chain of referred components with our own default
chain. We do this
+ * by prepending our own defaultFormatChain to the chain of referred
components. Prepends are
+ * efficient, but this creates the chain in the reverse order, since we want
to give
+ * precedence to the innermost referred components (see section 8.1.4 of the
DFDL
+ * specification). We could reverse it now, but the reversed list is
actually useful to keep
+ * around. For example, Daffodil 4.1.0 fixed a bug to correctly used the
non-reversed order,
+ * but some schemas relied on this buggy behavior. By storing the old
reversed order, we can
+ * later used this to detect and warn if schemas might rely on the buggy
behavior.
Review Comment:
later used => use
if schemas might rely on => if schemas are relying on
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/AnnotatedSchemaComponent.scala:
##########
@@ -357,16 +371,33 @@ trait AnnotatedSchemaComponent
}
}.value
- final protected lazy val defaultPropertySources: Seq[ChainPropProvider] =
- LV(Symbol("defaultPropertySources")) {
+ /**
+ * The default property sources we should use for resolving property values
is calculated by
+ * combining the default chain of referred components with our own default
chain. We do this
+ * by prepending our own defaultFormatChain to the chain of referred
components. Prepends are
+ * efficient, but this creates the chain in the reverse order, since we want
to give
+ * precedence to the innermost referred components (see section 8.1.4 of the
DFDL
+ * specification). We could reverse it now, but the reversed list is
actually useful to keep
+ * around. For example, Daffodil 4.1.0 fixed a bug to correctly used the
non-reversed order,
Review Comment:
used => use
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/AnnotatedSchemaComponent.scala:
##########
@@ -107,10 +107,24 @@ trait ResolvesScopedProperties extends FindPropertyMixin
{ self: Term =>
private def findDefaultProperty(pname: String): PropertyLookupResult = {
val result = findPropertyInSources(pname, defaultPropertySources)
val fixup = result match {
- case Found(value, loc, pname, _) =>
+ case Found(value, loc, pname, _) => {
+ // we found the property in default format annotation. Daffodil 4.1.0
reversed the order
Review Comment:
reversed => "corrected"
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/AnnotatedSchemaComponent.scala:
##########
@@ -107,10 +107,24 @@ trait ResolvesScopedProperties extends FindPropertyMixin
{ self: Term =>
private def findDefaultProperty(pname: String): PropertyLookupResult = {
val result = findPropertyInSources(pname, defaultPropertySources)
val fixup = result match {
- case Found(value, loc, pname, _) =>
+ case Found(value, loc, pname, _) => {
+ // we found the property in default format annotation. Daffodil 4.1.0
reversed the order
+ // we resolved default property sources to correctly match the DFDL
specification.
+ // Fortunately, this does not affect most schemas, but for those that
it does, it can be
+ // very difficult to determine what the old value was. So we lookup
property sources in
+ // the old incorrect reversed and see if we find the same value and
warn if not
+ val oldResult = findPropertyInSources(pname,
defaultPropertySourcesReversed)
+ val oldValue = oldResult.toOption.get
+ schemaDefinitionWarningWhen(
+ WarnID.ChangedDefaultPropertyResolution,
+ oldValue != value,
+ s"""Value of property $pname changed from "$oldValue" to "$value"
due to changes in default property resolution."""
Review Comment:
changes in => corrections to
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
