You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "stevedlawrence (via GitHub)" <gi...@apache.org> on 2024/09/24 13:16:23 UTC
[PR] Fix how unprefixed QNames are resolved [daffodil]
stevedlawrence opened a new pull request, #1314:
URL: https://github.com/apache/daffodil/pull/1314
Commit 2590f54ce9 change it so that if no default namespace is defined then an unprefixed QName reference is resolved using the targetNamespace as its namespace. However, this logic was not correct.
Instead, we need different logic depending on if the schema containing such a reference is included vs imported. In the former case, we should use the targetNamespace of the including schema. In the latter, we should just use no-namespace.
This modifies the logic to add a new 'noPrefixNamespace' member which keeps track of what namespace to use when resolving a unprefixed QName reference using include/import information. This member replaces previous changes of targetNamespace.
This found a number of tests where references to defined formats were not actually correct but happened to work due to previously flawed logic. This corrects these tests, and adds additional tests to make sure include/import logic works as expected.
Deprecation/Compatibility:
A bug was fixed so Daffodil now correctly handles resolving unprefixed QName references. However, this means that some unprefixed references that relied on the previous incorrect behavior may not resolve anymore. Although there are a number of ways to fix this, defining a namespace prefix and adding that prefix to broken reference should work in all cases.
DAFFODIL-2961
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix how unprefixed QNames are resolved [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1314:
URL: https://github.com/apache/daffodil/pull/1314#discussion_r1773481184
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/xml/QNameBase.scala:
##########
@@ -581,9 +585,9 @@ object RefQNameFactory extends RefQNameFactoryBase[RefQName] {
val resolvedNS = (preNS, ns) match {
case (None, UnspecifiedNamespace) => {
// if neither prefix nor namespace was specified, use the default namespace (i.e.
- // xmln="...") if defined. If not defined, use the targetNamespace.
+ // xmlns="...") if defined. If not defined, use the noPrefixNamespace.
val defaultNS = resolveDefaultNamespace(scope, unqualifiedPathStepPolicy)
- defaultNS.map(NS(_)).getOrElse(targetNamespace)
+ defaultNS.getOrElse(noPrefixNamespace)
Review Comment:
Feels like a case that needs a covering test.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/SchemaDocIncludesAndImportsMixin.scala:
##########
@@ -93,6 +93,39 @@ trait SchemaDocIncludesAndImportsMixin { self: XMLSchemaDocument =>
resultNS
}.value
+ /**
+ * when we resolve a QName reference, if that reference does not have a prefix and there is no
+ * in-scope default namespace, then we use this namespace, which varies depending on things
+ * like targetNamespace and whether this schema was included or imported
+ */
+ override lazy val noPrefixNamespace: NS = LV('noPrefixNamespace) {
+ ii match {
+ case Some(inc: Include) => {
+ // if this schema document was included in another document, then either the two
+ // schemas already have the same targetNamespace or this schema has no-namespace and
+ // is chameleoned into the targetNamespace of the including schema. Either way, the
+ // resulting included elements are in the targetNamespace and so any unprefixed
+ // references to those elements should use the including schemas targetNamespace when
+ // resolving
+ inc.targetNamespace
+ }
+ case Some(imp: Import) => {
+ // if this schema document was imported and we don't have a default namespace then any
+ // unprefixed references just resolve to NoNamespace. Note that if this schema has a
+ // targetNamespace, then it can only reference elements that are imported into it
+ // without a namespace
+ NoNamespace
+ }
+ case Some(_) => Assert.impossible()
+ case None => {
+ // if we weren't included or imported it means we are the bootstrap schema. There are
+ // no references in a bootstrap schema, only an import of the real schema, so this
+ // value doesn't really matter and should never be used.
+ NoNamespace
Review Comment:
If it "should never be used" I'd expect codecov to tell us it is uncovered. But it doesn't. So I suspect the comment "should never be used" isn't quite right.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/GrammarTerm.scala:
##########
@@ -56,7 +56,7 @@ abstract class Gram(contextArg: SchemaComponent)
final override lazy val tunable = context.tunable
final override def namespaces = context.namespaces
- final override def targetNamespace = context.targetNamespace
+ final override def noPrefixNamespace = context.noPrefixNamespace
Review Comment:
So this means at the level of Gram objects, nobody ever asks for noPrefixNamespace. At least in our tests.
We should either have a test that exercises this, or we should remove the member. It's likely this is just bypassed, as in the places that could call it are themselves calling noPrefixNamespace on the context object themselves.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/xml/QNameBase.scala:
##########
@@ -168,19 +168,22 @@ object QName {
def resolveStep(
qnameString: String,
scope: scala.xml.NamespaceBinding,
- targetNamespace: NS,
+ noPrefixNamespace: NS,
unqualifiedPathStepPolicy: UnqualifiedPathStepPolicy
): Try[StepQName] = {
- // TODO: We pass in NoNamespace instead of targetNamespace here, which may not be correct in
- // all cases. If the step being referenced is a global element or if elementFormDefault is
- // qualified then we probably do want to use targetNamespace to look it up since those
- // elements likely have a namespace. But for non-qualified local steps, we probably do just
- // want to use NoNamespace since the step won't have a namespace. However, this is sort of a
- // chicken-and-egg problem--we need to know the namespace to how to find the step, but we
- // don't know which namespace to use (NoNamespace or targetNamespace) unless we've already
- // found the same. Instead, we use NoNamespace and let the flexibility that
- // unqualfiedPathStepPolicy gives use help to find the step element. Possibly related to
- // DAFFODIL-2917
+ // It is not clear what namespace to use when a path step does not have a prefix and there
+ // is no default namespace defined. The step could be to a global element in the same
Review Comment:
Comment isn't right. Or reflects the confusion that this fix straightens out.
The reasoning is expressed in the definition of the noPrefixNamespace member.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/xml/QNameBase.scala:
##########
@@ -518,28 +521,25 @@ protected trait RefQNameFactoryBase[T] {
def resolveRef(
Review Comment:
Suggest improve the scaladoc above because this code is handling both prefixed and non-prefixed refs. The existing scaladoc suggests this is only about QNames with an explicit prefix.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix how unprefixed QNames are resolved [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1314:
URL: https://github.com/apache/daffodil/pull/1314#discussion_r1773804408
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/RuntimeData.scala:
##########
@@ -659,6 +659,7 @@ sealed class ElementRuntimeData(
override val minimizedScope: NamespaceBinding,
defaultBitOrderArg: BitOrder,
val optPrimType: Option[PrimType],
+ val targetNamespace: NS,
Review Comment:
Same as above, this is just revertting a previous change to get the original `targetNamespace` back to where it was.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix how unprefixed QNames are resolved [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1314:
URL: https://github.com/apache/daffodil/pull/1314#discussion_r1773761002
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/xml/QNameBase.scala:
##########
@@ -168,19 +168,22 @@ object QName {
def resolveStep(
qnameString: String,
scope: scala.xml.NamespaceBinding,
- targetNamespace: NS,
+ noPrefixNamespace: NS,
unqualifiedPathStepPolicy: UnqualifiedPathStepPolicy
): Try[StepQName] = {
- // TODO: We pass in NoNamespace instead of targetNamespace here, which may not be correct in
- // all cases. If the step being referenced is a global element or if elementFormDefault is
- // qualified then we probably do want to use targetNamespace to look it up since those
- // elements likely have a namespace. But for non-qualified local steps, we probably do just
- // want to use NoNamespace since the step won't have a namespace. However, this is sort of a
- // chicken-and-egg problem--we need to know the namespace to how to find the step, but we
- // don't know which namespace to use (NoNamespace or targetNamespace) unless we've already
- // found the same. Instead, we use NoNamespace and let the flexibility that
- // unqualfiedPathStepPolicy gives use help to find the step element. Possibly related to
- // DAFFODIL-2917
+ // It is not clear what namespace to use when a path step does not have a prefix and there
+ // is no default namespace defined. The step could be to a global element in the same
Review Comment:
Withdrawn.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/GrammarTerm.scala:
##########
@@ -56,7 +56,7 @@ abstract class Gram(contextArg: SchemaComponent)
final override lazy val tunable = context.tunable
final override def namespaces = context.namespaces
- final override def targetNamespace = context.targetNamespace
+ final override def noPrefixNamespace = context.noPrefixNamespace
Review Comment:
This makes sense to me. DSOM should be the only thing that knows anything about namespaces. That said, some stuff happens at Gram level that probably should not.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix how unprefixed QNames are resolved [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1314:
URL: https://github.com/apache/daffodil/pull/1314#discussion_r1773771199
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/GrammarTerm.scala:
##########
@@ -56,7 +56,7 @@ abstract class Gram(contextArg: SchemaComponent)
final override lazy val tunable = context.tunable
final override def namespaces = context.namespaces
- final override def targetNamespace = context.targetNamespace
+ final override def noPrefixNamespace = context.noPrefixNamespace
Review Comment:
I was able to remove ResolvesQNames from the Gram level without issue, it was a relatively small change.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix how unprefixed QNames are resolved [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence merged PR #1314:
URL: https://github.com/apache/daffodil/pull/1314
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix how unprefixed QNames are resolved [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on PR #1314:
URL: https://github.com/apache/daffodil/pull/1314#issuecomment-2371474096
I think the ticket is DAFFODIL-2916, not 2961.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix how unprefixed QNames are resolved [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1314:
URL: https://github.com/apache/daffodil/pull/1314#discussion_r1773803872
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/ElementBaseRuntime1Mixin.scala:
##########
@@ -191,6 +191,7 @@ trait ElementBaseRuntime1Mixin { self: ElementBase =>
minimizedScope,
defaultBitOrder,
optPrimType,
+ targetNamespace,
Review Comment:
This was removed by the original patch the avoid duplicating `targetNamespace` in the runtime data here:
https://github.com/apache/daffodil/commit/2590f54ce93e262d02660bd9d57bc7979a7d06d9#diff-65004632a05e8533fdb90c6f2fc0d9e840ffa93293ae171e54e1cd99591bc540L657
This PR replaces `targetNamespace` with `noPrefixNamespace` but we still need the original `targetNamespace`, so this change essentially reverts the original change back to where `targetNamespace` was.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix how unprefixed QNames are resolved [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1314:
URL: https://github.com/apache/daffodil/pull/1314#discussion_r1773625884
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/xml/QNameBase.scala:
##########
@@ -168,19 +168,22 @@ object QName {
def resolveStep(
qnameString: String,
scope: scala.xml.NamespaceBinding,
- targetNamespace: NS,
+ noPrefixNamespace: NS,
unqualifiedPathStepPolicy: UnqualifiedPathStepPolicy
): Try[StepQName] = {
- // TODO: We pass in NoNamespace instead of targetNamespace here, which may not be correct in
- // all cases. If the step being referenced is a global element or if elementFormDefault is
- // qualified then we probably do want to use targetNamespace to look it up since those
- // elements likely have a namespace. But for non-qualified local steps, we probably do just
- // want to use NoNamespace since the step won't have a namespace. However, this is sort of a
- // chicken-and-egg problem--we need to know the namespace to how to find the step, but we
- // don't know which namespace to use (NoNamespace or targetNamespace) unless we've already
- // found the same. Instead, we use NoNamespace and let the flexibility that
- // unqualfiedPathStepPolicy gives use help to find the step element. Possibly related to
- // DAFFODIL-2917
+ // It is not clear what namespace to use when a path step does not have a prefix and there
+ // is no default namespace defined. The step could be to a global element in the same
Review Comment:
Can you clarify what isn't right about this comment? My goal with this is to explain why we don't use the `noPrefixNamespace` parameter when calling resolveRef and instead pass in `NoNamespace`.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix how unprefixed QNames are resolved [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1314:
URL: https://github.com/apache/daffodil/pull/1314#discussion_r1773679415
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/GrammarTerm.scala:
##########
@@ -56,7 +56,7 @@ abstract class Gram(contextArg: SchemaComponent)
final override lazy val tunable = context.tunable
final override def namespaces = context.namespaces
- final override def targetNamespace = context.targetNamespace
+ final override def noPrefixNamespace = context.noPrefixNamespace
Review Comment:
It looks like not only does nothing ask for `noPrefixNamespace` at the level of `Gram` objects, nothing also asks for `namespaces` and `unqualifiedPathStep` policy, two things also needed for resolving references. It looks like codecov only sees no coverage on noPrefixNamespace for some reason.
I think what that means is we need to move `ResolvesQNames` to a trait down a level so that is only on a DSOM objects but not any Gram objects. Which I think makes sense? DSOM should be resolving everything and then Grams are created based on those results.
I think that means `ResolvesQNames` should be on `SchemaComponent` instead. I'll move it and see what happens.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix how unprefixed QNames are resolved [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1314:
URL: https://github.com/apache/daffodil/pull/1314#discussion_r1773612882
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/SchemaDocIncludesAndImportsMixin.scala:
##########
@@ -93,6 +93,39 @@ trait SchemaDocIncludesAndImportsMixin { self: XMLSchemaDocument =>
resultNS
}.value
+ /**
+ * when we resolve a QName reference, if that reference does not have a prefix and there is no
+ * in-scope default namespace, then we use this namespace, which varies depending on things
+ * like targetNamespace and whether this schema was included or imported
+ */
+ override lazy val noPrefixNamespace: NS = LV('noPrefixNamespace) {
+ ii match {
+ case Some(inc: Include) => {
+ // if this schema document was included in another document, then either the two
+ // schemas already have the same targetNamespace or this schema has no-namespace and
+ // is chameleoned into the targetNamespace of the including schema. Either way, the
+ // resulting included elements are in the targetNamespace and so any unprefixed
+ // references to those elements should use the including schemas targetNamespace when
+ // resolving
+ inc.targetNamespace
+ }
+ case Some(imp: Import) => {
+ // if this schema document was imported and we don't have a default namespace then any
+ // unprefixed references just resolve to NoNamespace. Note that if this schema has a
+ // targetNamespace, then it can only reference elements that are imported into it
+ // without a namespace
+ NoNamespace
+ }
+ case Some(_) => Assert.impossible()
+ case None => {
+ // if we weren't included or imported it means we are the bootstrap schema. There are
+ // no references in a bootstrap schema, only an import of the real schema, so this
+ // value doesn't really matter and should never be used.
+ NoNamespace
Review Comment:
It looks like codecov doesn't understand this code and can't properly detect that there is no coverage. I've confirmed that this bit of code is never hit. I'll change it to an Assert.impossible and update the comment.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix how unprefixed QNames are resolved [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1314:
URL: https://github.com/apache/daffodil/pull/1314#discussion_r1773643459
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/xml/QNameBase.scala:
##########
@@ -581,9 +585,9 @@ object RefQNameFactory extends RefQNameFactoryBase[RefQName] {
val resolvedNS = (preNS, ns) match {
case (None, UnspecifiedNamespace) => {
// if neither prefix nor namespace was specified, use the default namespace (i.e.
- // xmln="...") if defined. If not defined, use the targetNamespace.
+ // xmlns="...") if defined. If not defined, use the noPrefixNamespace.
val defaultNS = resolveDefaultNamespace(scope, unqualifiedPathStepPolicy)
- defaultNS.map(NS(_)).getOrElse(targetNamespace)
+ defaultNS.getOrElse(noPrefixNamespace)
Review Comment:
This entire function has zero coverage and is not used anywhere. This is just dead code that I think we should probably just remove.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix how unprefixed QNames are resolved [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1314:
URL: https://github.com/apache/daffodil/pull/1314#discussion_r1773788546
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/ElementBaseRuntime1Mixin.scala:
##########
@@ -191,6 +191,7 @@ trait ElementBaseRuntime1Mixin { self: ElementBase =>
minimizedScope,
defaultBitOrder,
optPrimType,
+ targetNamespace,
Review Comment:
Should this be using noPrefixNamespace as well?
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/RuntimeData.scala:
##########
@@ -659,6 +659,7 @@ sealed class ElementRuntimeData(
override val minimizedScope: NamespaceBinding,
defaultBitOrderArg: BitOrder,
val optPrimType: Option[PrimType],
+ val targetNamespace: NS,
Review Comment:
Same question here
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org