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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
