You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by GitBox <gi...@apache.org> on 2021/03/08 13:19:02 UTC
[GitHub] [daffodil] scala-steward opened a new pull request #505: Update scala-library, scala-reflect to 2.12.13
scala-steward opened a new pull request #505:
URL: https://github.com/apache/daffodil/pull/505
Updates
* [org.scala-lang:scala-library](https://github.com/scala/scala.git)
* [org.scala-lang:scala-reflect](https://github.com/scala/scala.git)
from 2.12.11 to 2.12.13.
I'll automatically update this PR to resolve conflicts as long as you don't change it yourself.
If you'd like to skip this version, you can just close this PR. If you have any feedback, just mention me in the comments below.
Configure Scala Steward for your repository with a [`.scala-steward.conf`](https://github.com/scala-steward-org/scala-steward/blob/7949462a266772e2dbf2718b5b3ddb332f0165c4/docs/repo-specific-configuration.md) file.
Have a fantastic day writing Scala!
<details>
<summary>Files still referring to the old version number</summary>
The following files still refer to the old version number (2.12.11).
You might want to review and update them manually.
```
daffodil-cli/bin.NOTICE
```
</details>
<details>
<summary>Ignore future updates</summary>
Add this to your `.scala-steward.conf` file to ignore future updates of this dependency:
```
updates.ignore = [ { groupId = "org.scala-lang" } ]
```
</details>
labels: library-update, semver-patch, old-version-remains
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] tuxji commented on pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
tuxji commented on pull request #505:
URL: https://github.com/apache/daffodil/pull/505#issuecomment-817829298
Also noticed a sbt warning. Can't find "coverageEnabled" anywhere in daffodil repository but found it in sbt-codecov repository.
```log
[info] Reapplying settings...
[info] set current project to daffodil (in build file:/home/runner/work/daffodil/daffodil/)
<set>:1: warning: method in in trait ScopingSetting is deprecated (since 1.5.0): `in` is deprecated; migrate to slash syntax - https://www.scala-sbt.org/1.x/docs/Migrating-from-sbt-013x.html#slash
coverageEnabled in ThisBuild := true
^
[info] Defining ThisBuild / coverageEnabled
[info] The new value will be used by Compile / compile / scalacOptions, daffodil-cli / Compile / compile / scalacOptions and 35 others.
[info] Run `last` for details.
[info] Reapplying settings...
```
Have filed an [issue](https://github.com/alejandrohdezma/sbt-codecov/issues/114) there asking maintainer to fix that warning.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #505:
URL: https://github.com/apache/daffodil/pull/505#issuecomment-829285780
My idea of duplicating the list doesn't actually work, so I'm wondering if there's something else going. Also, I think scala 2.13 has removed this SerializationProxy thing for Lists, so maybe they consider it fixed in that release.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #505:
URL: https://github.com/apache/daffodil/pull/505#discussion_r624122775
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/SAXInfosetOutputter.scala
##########
@@ -145,18 +145,11 @@ class SAXInfosetOutputter(xmlReader: DFDL.DaffodilParseXMLReader,
private def doStartPrefixMapping(diElem: DIElement, contentHandler: ContentHandler): Unit = {
val (nsbStart: NamespaceBinding, nsbEnd: NamespaceBinding) = getNsbStartAndEnd(diElem)
var n = nsbStart
- var mappingsList: Seq[(String, String)] = Seq()
while (n.ne(nsbEnd) && n.ne(null) && n.ne(scala.xml.TopScope)) {
val prefix = if (n.prefix == null) "" else n.prefix
val uri = if (n.uri == null) "" else n.uri
- // we generate a list here by prepending so can build the prefixMapping in the
- // same order as it is in minimizedScope when we call startPrefixMapping; we do this because
- // getPrefix in the contentHandler is order dependent
- mappingsList +:= (prefix, uri)
- n = n.parent
- }
- mappingsList.foreach{ case (prefix, uri) =>
contentHandler.startPrefixMapping(prefix, uri)
+ n = n.parent
Review comment:
Yep, your mental parse is correct.
> But to any other content handler what was a(b(c)) in the minimized scope would now have become c(b(a)), no?
I think the answer to this is in most cases it doesn't matter, and if it does it's up to the ContentHandler to decide. Here's my thought process:
If the minimizedScope was a(b(c)), to Daffodil that reprents bindings in this order when converted to text:
```xml
<a xmlns:a="1" xmlns:b="2" xmlns:c="3" />
```
And so we will call startPrefixMapping in the order of a, b, c. This matces the order that the xmlns appears in the text representation. And I think that's all that matters. How a ContentHandler interprets these mapping and uses them for prefix lookups or converting to text is entirely up to them. Keep in mind they don't have to use NamespaceBindings. Maybe they use a stack of Maps or some complicated data structure. All we are really saying by calling startPrefixMapping is "this is the order we think the xmlns mappings appear in the text representation, you figure out the rest".
And in most cases, order probably doesn't even matter. If they end up creating a NamespaceBinding that is c(b(a)) and do prefix lookups on that, the results will be exactly the same. This is because order doesn't matter in namespace bindings in relation to a single element (*see potential issue at bottom*). For example this is exactly the same as the above XML from a prefix lookup perspective:
```xml
<a xmlns:c="3" xmlns:b="2" xmlns:c="1" />
```
Regardless of the order of those mappings, the a, b, and c prefixes all map to the right uri. A ContentHandler could add those three to a NamespaceBinding (or any data structure of choosing) in any order and prefix lookups would work exactly the same (as long as they took precendece to any outer scope prefixes).
The issue that potentially pops up is if a ContentHandler wants to convert the prefix mappings to XML text, like our DaffodilParseOutputStream thing does. In that case, we might care about the order of things, because it is no longer just about mappings (where order doesn't matter), but is now about outputing XML text (where order does kindof matter because of our tests). So our content handler is careful to outut xmlns text in the same order it recieves the mappings using the new NamespaceBinding logic.
---
I think maybe the only case where order might matter with prefix look ups is if you had something like this:
```xml
<pre:a xmlns:pre="foo" xmlns:pre="bar" />
```
So you have the same prefix that maps to two diferent URIs. In that case, there are two potential namespace bindings depending on order:
```scala
NamespaceBinding("pre", "foo", NamespaceBinding("pre, "bar", null))
NamespaceBinding("pre", "bar", NamespaceBinding("pre, "foo", null))
```
So depending on which NamespaceBinding is used, a prefix lookup of "pre" could return either "foo" or "bar". But it feels to me like redefining a prefix on the same element shouldn't even be allowed--order isn't supposed to matter in prefix mappings, so if prefix redefinition on a single element was allowed, then order would be the only other thing to pick a winner. I would guess this is illegal and isn't even something Daffodil would ever do.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] olabusayoT commented on a change in pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
olabusayoT commented on a change in pull request #505:
URL: https://github.com/apache/daffodil/pull/505#discussion_r624071841
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/SAXInfosetOutputter.scala
##########
@@ -145,18 +145,11 @@ class SAXInfosetOutputter(xmlReader: DFDL.DaffodilParseXMLReader,
private def doStartPrefixMapping(diElem: DIElement, contentHandler: ContentHandler): Unit = {
val (nsbStart: NamespaceBinding, nsbEnd: NamespaceBinding) = getNsbStartAndEnd(diElem)
var n = nsbStart
- var mappingsList: Seq[(String, String)] = Seq()
while (n.ne(nsbEnd) && n.ne(null) && n.ne(scala.xml.TopScope)) {
val prefix = if (n.prefix == null) "" else n.prefix
val uri = if (n.uri == null) "" else n.uri
- // we generate a list here by prepending so can build the prefixMapping in the
- // same order as it is in minimizedScope when we call startPrefixMapping; we do this because
- // getPrefix in the contentHandler is order dependent
- mappingsList +:= (prefix, uri)
- n = n.parent
- }
- mappingsList.foreach{ case (prefix, uri) =>
contentHandler.startPrefixMapping(prefix, uri)
+ n = n.parent
Review comment:
Hmm let me mentally parse this out, so if we have the minimizedScope as mentioned above, with this new order, we'd call startPrefixMapping in the order below since c is parent of b, which is parent of a
```
contentHandler.startPrefixMapping("a", "baz")
contentHandler.startPrefixMapping("b", "bar")
contentHandler.startPrefixMapping("c", "foo")
```
that will cause currentPrefixMapping to look like:
`NamespaceBinding("c", "foo", NamespaceBinding("b", "bar", NamespaceBinding("a","baz", null)))`
this order is fixed when we convert to activePrefixMapping, which correctly reverses the order to a(b(c)), and our look ups are fine. But to any other content handler what was a(b(c)) in the minimized scope would now have become c(b(a)), no?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on a change in pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #505:
URL: https://github.com/apache/daffodil/pull/505#discussion_r623963468
##########
File path: daffodil-cli/src/it/resources/org/apache/daffodil/CLI/output/output9.txt
##########
@@ -1,4 +1,4 @@
-<base14:rabbitHole xmlns:b14="http://b14.com" xmlns:a14="http://a14.com" xmlns:base14="http://baseSchema.com">
+<base14:rabbitHole xmlns:a14="http://a14.com" xmlns:b14="http://b14.com" xmlns:base14="http://baseSchema.com">
Review comment:
Gotcha thanks. A comment to that effect with the test would be helpful.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #505:
URL: https://github.com/apache/daffodil/pull/505#issuecomment-829375313
Yep, I'll add the snippet above as a test that should a ClassCastException unless this is fixed.
Also looks like this identity stuff broke a couple SAX tests, need to track those down and see if maybe this isn't the appropriate fix...
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #505:
URL: https://github.com/apache/daffodil/pull/505#issuecomment-829218933
Finally figured out what's causing this one test to fail. It's actually caused by 2.12.12, though I'm not sure what changed from 2.12.11 to 2.12.12 to cause it to happen, but at least I know why it's failing, and some potential workarounds. Here's a really simple way to reproduce the failure:
```scala
// Thing contains list of other Things
class Thing() extends Serializable {
var things: Seq[Thing] = _
}
object Test {
def main(args: Array[String]): Unit = {
// Create two things
val a = new Thing()
val b = new Thing()
// Put thing B in a list
val l = Seq(b)
// Set that list to the things list in both A and B
a.things = l
b.things = l
// Serialize and Deserialize Thing A
val baos = new java.io.ByteArrayOutputStream()
val oos = new java.io.ObjectOutputStream(baos)
oos.writeObject(a)
oos.close()
baos.close()
val bytes = baos.toByteArray()
val bais = new java.io.ByteArrayInputStream(bytes)
val ois = new java.io.ObjectInputStream(bais)
val newThingA = ois.readObject().asInstanceOf[Thing]
}
}
```
So we have a list containing thing B, an both A and B have a reference to that list. And then we try to serialize and deserialize A.
Unfortunately, this fails on the last line when trying to deserialize with the error:
> java.lang.ClassCastException: cannot assign instance of scala.collection.immutable.List$SerializationProxy to field Thing.things of type scala.collection.Seq in instance of Thing
This happens because of the way that ``List``'s are serialized. Rather than serializing the ``List`` directly, Scala instead serializes a ``SerializationProxy``. When this is deserializes, Java first deserializes the ``SerializationProxy`` and then Scala converts that deserialized object back to a List. This issue is that with Java's default deserialization, extra deserialization stuff goes on in between this the deserialization of the proxy and converting it to a List, which causes issues. Below is a summary of the steps that happen when we try to deserialize A:
1. Deserialize A
1. Deserialize A.things
1. Create a SerializationProxy for A.things
1. Add the new SerializationProxy to a reference lookup table
1. Recursively deserialize the list contents
1. Deserialize B
1. Deserialize B.things
1. B.things is actually the same list reference that we added to the reference lookup table, so just set B.things to the value in that table, nothing extra to deserialize
1. B is done
1. The list contents are deserialized, build a new List from the deserialized SerializationProxy
1. Set A.things to that new List
1. Update the referenece lookup table to have the new List instead of the SerializationProxy
1. A.things is done
1. A is done
So the issue is that when B.things is set to the value in the lookup table, it is set to the SerializtionProxy beause it hasn't been replaced with the real List yet. This is where things fail because B.things is expecting to be set to a List and not a SerializationProxy, which is exactly what the error is saying.
This same kind of recursive back reference with shared Lists is exactly what's going on with the DPathElementCompileInfo (e.g ``Thing``) and the elementCompileInfos member (e.g. ``things``). We could maybe write custom serialization code to fix this, but I'm not sure I really want to maintain that. Maybe a simple alterantive is to just change elementCompileInfos so it isn't a List, but is instead an Array. This way we avoid this whole SerializationProxy stuff? Or I think we could clone the elementCompileInfos List. That way there would be no references to the same List, at the expense of extra memory usage.
Any other thoughts? Maybe we need to rethink the parent backpointer in DPathCompileInfo, which I think is the root of the problem?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #505:
URL: https://github.com/apache/daffodil/pull/505#issuecomment-829556519
Debugging the code, it looks like minimizedScope, which is based on ``Set``'s and ``Map``'s and don't have a well-defined order change from 2.12.11 to 2.12.13. We had tests that depended on this order, and it wouldn't surprise me if this we also depended on this order for the serialization to work.
I've updated minizedScope so that we are consistent with the ordering of NamespaceBindings.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] olabusayoT edited a comment on pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
olabusayoT edited a comment on pull request #505:
URL: https://github.com/apache/daffodil/pull/505#issuecomment-830228054
Correct me if I'm wrong, but these changes might mean our xmlreader cannot be used with another contentHandler and still give the right prefix mappings (when we do lookups). This might not be a problem for some users, but maybe we need to bundle the contentHandler with our J/SAPI, if people care about the prefixes?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] olabusayoT commented on pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
olabusayoT commented on pull request #505:
URL: https://github.com/apache/daffodil/pull/505#issuecomment-830228054
Correct me if I'm wrong, but these changes might mean our xmlreader cannot be used with another contentHandler and still give the right ordering for the prefix. This might not be a problem for some, but maybe be need to bundle the contentHandler with our J/SAPI?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence merged pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
stevedlawrence merged pull request #505:
URL: https://github.com/apache/daffodil/pull/505
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on pull request #505:
URL: https://github.com/apache/daffodil/pull/505#issuecomment-829370933
Can we add a test that creates the serialization failure in a small isolated example, so that in the future, when that test "breaks" because the problem is fixed we can consider removing these otherwise-unmotivated workarounds?
Also need comments in the code where these workarounds appear so we don't lose them, and partly so that if we add more objects like this we can copy the idiom.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence edited a comment on pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
stevedlawrence edited a comment on pull request #505:
URL: https://github.com/apache/daffodil/pull/505#issuecomment-830099505
Trying to fix this ordering issue revealed an ordering issues. It should all be fixed now. Everything is consistent and obeys the order of minimzedScope.
@olabusayoT please take a look, this tweaks the DaffodilParseContentHandler a bit
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #505:
URL: https://github.com/apache/daffodil/pull/505#issuecomment-830099505
Trying to fix this ordering issue revealed an ordering issues. It should all be fixed now. Everything is constent and obeys the order of minimzedScope.
@olabusayoT please take a look, this tweaks the DaffodilParseContentHandler a bit
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #505:
URL: https://github.com/apache/daffodil/pull/505#discussion_r623972525
##########
File path: daffodil-cli/src/it/resources/org/apache/daffodil/CLI/output/output9.txt
##########
@@ -1,4 +1,4 @@
-<base14:rabbitHole xmlns:b14="http://b14.com" xmlns:a14="http://a14.com" xmlns:base14="http://baseSchema.com">
+<base14:rabbitHole xmlns:a14="http://a14.com" xmlns:b14="http://b14.com" xmlns:base14="http://baseSchema.com">
Review comment:
Yep, very likely what happened.
I can't add a comment to the output file without breaking the test since that file is exactly the output of the CLI, but I've added a comment to the test that uses this file.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on pull request #505:
URL: https://github.com/apache/daffodil/pull/505#issuecomment-829269992
Serialization has to properly handle cyclic structures of any kind/shape or it is simply broken, and we should not upgrade to these new versions and should report a severe/critical bug.
I'm not opposed to a work-around like using a non-list collection type, but designing around cyclic structures because serialization could be flakey is unstable.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #505:
URL: https://github.com/apache/daffodil/pull/505#discussion_r624047759
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/SAXInfosetOutputter.scala
##########
@@ -145,18 +145,11 @@ class SAXInfosetOutputter(xmlReader: DFDL.DaffodilParseXMLReader,
private def doStartPrefixMapping(diElem: DIElement, contentHandler: ContentHandler): Unit = {
val (nsbStart: NamespaceBinding, nsbEnd: NamespaceBinding) = getNsbStartAndEnd(diElem)
var n = nsbStart
- var mappingsList: Seq[(String, String)] = Seq()
while (n.ne(nsbEnd) && n.ne(null) && n.ne(scala.xml.TopScope)) {
val prefix = if (n.prefix == null) "" else n.prefix
val uri = if (n.uri == null) "" else n.uri
- // we generate a list here by prepending so can build the prefixMapping in the
- // same order as it is in minimizedScope when we call startPrefixMapping; we do this because
- // getPrefix in the contentHandler is order dependent
- mappingsList +:= (prefix, uri)
- n = n.parent
- }
- mappingsList.foreach{ case (prefix, uri) =>
contentHandler.startPrefixMapping(prefix, uri)
+ n = n.parent
Review comment:
Hmm, I'm not sure. Imagine our namespace prefixes for an element were this (in no particular order):
* ``xmlns:a="foo"``
* ``xmlns:b="bar"``
* ``xmlns:c="baz"``
With the new sorting, minizedScope for the above would like this:
```scala
NamespaceBinding("a", "foo", NamespaceBinding("b", "bar", NamespaceBinding("c","baz", null)))
```
When we do a toString on this (e.g. what XMLTextInfoOutputter and others do), this will be output to the string:
```
xmlns:a="foo" xmlns:b="bar" xmlns:c="baz"
```
So everything is in the expected sorted order.
But in the above before thsi change, we called startPrefixMapping in the reverse order than they were in minimizedScope. So with the above NamespaceBinding, I think we would call this before:
```scala
contentHandler.startPrefixMapping("c", "baz")
contentHandler.startPrefixMapping("b", "bar")
contentHandler.startPrefixMapping("a", "foo")
```
Which means the ContentHandler got the mappings in the reverse order than they should show up in the XML.
I think the DaffodilParseOutputStreamContentHandler expected this reverse order and then effecitvely re-reversed things when it added them to the currentElement/activePrefixMapping so things showed up correct. But I think maybe that was incorrect? With these changes, startPrefixMapping is called in the same order as the minimizedScope, and then the new logic in the DaffodilParseOutputStreamContentHandler ensures things are written to the Writer in the correct order.
So I *think* this actually fixes the order that we call startPrefixMapping to match minimzedScope? And perhaps they would have showed up backwards to other ContentHandlers?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #505:
URL: https://github.com/apache/daffodil/pull/505#discussion_r624122775
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/SAXInfosetOutputter.scala
##########
@@ -145,18 +145,11 @@ class SAXInfosetOutputter(xmlReader: DFDL.DaffodilParseXMLReader,
private def doStartPrefixMapping(diElem: DIElement, contentHandler: ContentHandler): Unit = {
val (nsbStart: NamespaceBinding, nsbEnd: NamespaceBinding) = getNsbStartAndEnd(diElem)
var n = nsbStart
- var mappingsList: Seq[(String, String)] = Seq()
while (n.ne(nsbEnd) && n.ne(null) && n.ne(scala.xml.TopScope)) {
val prefix = if (n.prefix == null) "" else n.prefix
val uri = if (n.uri == null) "" else n.uri
- // we generate a list here by prepending so can build the prefixMapping in the
- // same order as it is in minimizedScope when we call startPrefixMapping; we do this because
- // getPrefix in the contentHandler is order dependent
- mappingsList +:= (prefix, uri)
- n = n.parent
- }
- mappingsList.foreach{ case (prefix, uri) =>
contentHandler.startPrefixMapping(prefix, uri)
+ n = n.parent
Review comment:
Yep, your mental parse is correct.
> But to any other content handler what was a(b(c)) in the minimized scope would now have become c(b(a)), no?
I think the answer to this is in most cases it doesn't matter, and if it does it's up to the ContentHandler to decide. Here's my thought process:
If the minimizedScope was a(b(c)), to Daffodil that reprents bindings in this order when converted to text:
```xml
<a xmlns:a="1" xmlns:b="2" xmlns:c="3" />
```
And so we will call startPrefixMapping in the order of a, b, c. This matces the order that the xmlns appears in the text representation. And I think that's all that matters. How a ContentHandler interprets these mapping and uses them for prefix lookups or converting to text is entirely up to them. Keep in mind they don't have to use NamespaceBindings. Maybe they use a stack of Maps or some complicated data structure. All we are really saying by calling startPrefixMapping is "this is the order we think the xmlns mappings appear in the text representation, you figure out the rest".
And in most cases, order probably doesn't even matter. If they end up creating a NamespaceBinding that is c(b(a)) and do prefix lookups on that, the results will be exactly the same. This is because order doesn't matter in namespace bindings in relation to a single element (*see potential issue at bottom*). For example this is exactly the same as the above XML from a prefix lookup perspective:
```xml
<a xmlns:c="3" xmlns:b="2" xmlns:c="1" />
```
Regardless of the order of those mappings, the a, b, and c prefixes all map to the right uri. A ContentHandler could add those three to a NamespaceBinding (or any data structure of choosing) in any order and prefix lookups would work exactly the same (as long as they took precendece to any outer scope prefixes).
The issue that potentially pops up is if a ContentHandler wants to convert the prefix mappings to XML text, like our DaffodilParseOutputStream thing does. In that case, we might care about the order of things, because it is no longer just about mappings (where order doesn't matter), but is now about outputing XML text (where order does kindof matter because of our tests). So our content handler is careful to outut xmlns text in the same order it recieves the mappings using the new NamespaceBinding.
---
I think maybe the only case where order might matter with prefix look ups is if you had something like this:
```xml
<pre:a xmlns:pre="foo" xmlns:pre="bar" />
```
So you have the same prefix that maps to two diferent URIs. In that case, there are two potential namespace bindings depending on order:
```scala
NamespaceBinding("pre", "foo", NamespaceBinding("pre, "bar", null))
NamespaceBinding("pre", "bar", NamespaceBinding("pre, "foo", null))
```
So depending on which NamespaceBinding is used, a prefix lookup of "pre" could return either "foo" or "bar". But it feels to me like redefining a prefix on the same element shouldn't even be allowed--order isn't supposed to matter in prefix mappings, so if prefix redefinition on a single element was allowed, then order would be the only other thing to pick a winner. I would guess this is illegal and isn't even something Daffodil would ever do.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #505:
URL: https://github.com/apache/daffodil/pull/505#discussion_r624167614
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/SAXInfosetOutputter.scala
##########
@@ -145,18 +145,11 @@ class SAXInfosetOutputter(xmlReader: DFDL.DaffodilParseXMLReader,
private def doStartPrefixMapping(diElem: DIElement, contentHandler: ContentHandler): Unit = {
val (nsbStart: NamespaceBinding, nsbEnd: NamespaceBinding) = getNsbStartAndEnd(diElem)
var n = nsbStart
- var mappingsList: Seq[(String, String)] = Seq()
while (n.ne(nsbEnd) && n.ne(null) && n.ne(scala.xml.TopScope)) {
val prefix = if (n.prefix == null) "" else n.prefix
val uri = if (n.uri == null) "" else n.uri
- // we generate a list here by prepending so can build the prefixMapping in the
- // same order as it is in minimizedScope when we call startPrefixMapping; we do this because
- // getPrefix in the contentHandler is order dependent
- mappingsList +:= (prefix, uri)
- n = n.parent
- }
- mappingsList.foreach{ case (prefix, uri) =>
contentHandler.startPrefixMapping(prefix, uri)
+ n = n.parent
Review comment:
Looking at the XML spec, it does look like that last scenario is not allowed. From https://www.w3.org/TR/xml-names/#uniqAttrs:
> In XML documents conforming to this specification, no tag may contain two attributes which:
> 1. have identical names, or
> 2. have qualified names with the same local part and with prefixes which have been bound to namespace names that are identical.
This doesn't specifically call out xmlns attribues which are a little special, but it seems reasonable that they must follow the same restrictions as normal attributes and can't be redeclared.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] olabusayoT commented on a change in pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
olabusayoT commented on a change in pull request #505:
URL: https://github.com/apache/daffodil/pull/505#discussion_r624071841
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/SAXInfosetOutputter.scala
##########
@@ -145,18 +145,11 @@ class SAXInfosetOutputter(xmlReader: DFDL.DaffodilParseXMLReader,
private def doStartPrefixMapping(diElem: DIElement, contentHandler: ContentHandler): Unit = {
val (nsbStart: NamespaceBinding, nsbEnd: NamespaceBinding) = getNsbStartAndEnd(diElem)
var n = nsbStart
- var mappingsList: Seq[(String, String)] = Seq()
while (n.ne(nsbEnd) && n.ne(null) && n.ne(scala.xml.TopScope)) {
val prefix = if (n.prefix == null) "" else n.prefix
val uri = if (n.uri == null) "" else n.uri
- // we generate a list here by prepending so can build the prefixMapping in the
- // same order as it is in minimizedScope when we call startPrefixMapping; we do this because
- // getPrefix in the contentHandler is order dependent
- mappingsList +:= (prefix, uri)
- n = n.parent
- }
- mappingsList.foreach{ case (prefix, uri) =>
contentHandler.startPrefixMapping(prefix, uri)
+ n = n.parent
Review comment:
Hmm let me mentally parse this out, so if we have the minimizedScope as mentioned above, with this new code, we'd call startPrefixMapping in the order below since c is parent of b, which is parent of a
```
contentHandler.startPrefixMapping("a", "baz")
contentHandler.startPrefixMapping("b", "bar")
contentHandler.startPrefixMapping("c", "foo")
```
that will cause currentPrefixMapping to look like:
`NamespaceBinding("c", "foo", NamespaceBinding("b", "bar", NamespaceBinding("a","baz", null)))`
this order is fixed when we convert to activePrefixMapping, which correctly reverses the order to a(b(c)), and our look ups are fine. But to any other content handler what was a(b(c)) in the minimized scope would now have become c(b(a)), no?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on a change in pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #505:
URL: https://github.com/apache/daffodil/pull/505#discussion_r623946100
##########
File path: daffodil-cli/src/it/resources/org/apache/daffodil/CLI/output/output9.txt
##########
@@ -1,4 +1,4 @@
-<base14:rabbitHole xmlns:b14="http://b14.com" xmlns:a14="http://a14.com" xmlns:base14="http://baseSchema.com">
+<base14:rabbitHole xmlns:a14="http://a14.com" xmlns:b14="http://b14.com" xmlns:base14="http://baseSchema.com">
Review comment:
Why does this matter? Order shouldn't matter for these.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #505:
URL: https://github.com/apache/daffodil/pull/505#discussion_r623964227
##########
File path: daffodil-cli/src/it/resources/org/apache/daffodil/CLI/output/output9.txt
##########
@@ -1,4 +1,4 @@
-<base14:rabbitHole xmlns:b14="http://b14.com" xmlns:a14="http://a14.com" xmlns:base14="http://baseSchema.com">
+<base14:rabbitHole xmlns:a14="http://a14.com" xmlns:b14="http://b14.com" xmlns:base14="http://baseSchema.com">
Review comment:
As to you other question, I'm not really sure why the serialization changes causes namespace mapping ordering changes. I suspect copying the list caused some things to be evaluated in a slightly different order and because we used Set/Maps, however things got lazily evaluated changed the order. It's not entirely clear to me, and I couldn't track down the underlying change. But 2.12.13 did backport some Map changes from 2.13.x, so a change in ordering isn't too surprising.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #505:
URL: https://github.com/apache/daffodil/pull/505#discussion_r624122775
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/SAXInfosetOutputter.scala
##########
@@ -145,18 +145,11 @@ class SAXInfosetOutputter(xmlReader: DFDL.DaffodilParseXMLReader,
private def doStartPrefixMapping(diElem: DIElement, contentHandler: ContentHandler): Unit = {
val (nsbStart: NamespaceBinding, nsbEnd: NamespaceBinding) = getNsbStartAndEnd(diElem)
var n = nsbStart
- var mappingsList: Seq[(String, String)] = Seq()
while (n.ne(nsbEnd) && n.ne(null) && n.ne(scala.xml.TopScope)) {
val prefix = if (n.prefix == null) "" else n.prefix
val uri = if (n.uri == null) "" else n.uri
- // we generate a list here by prepending so can build the prefixMapping in the
- // same order as it is in minimizedScope when we call startPrefixMapping; we do this because
- // getPrefix in the contentHandler is order dependent
- mappingsList +:= (prefix, uri)
- n = n.parent
- }
- mappingsList.foreach{ case (prefix, uri) =>
contentHandler.startPrefixMapping(prefix, uri)
+ n = n.parent
Review comment:
Yep, your mental parse is correct.
> But to any other content handler what was a(b(c)) in the minimized scope would now have become c(b(a)), no?
I think the answer to this is in most cases it doesn't matter, and if it does it's up to the ContentHandler to decide. Here's my thought process:
If the minimizedScope was a(b(c)), to Daffodil that reprents bindings in this order when converted to text:
```xml
<a xmlns:a="1" xmlns:b="2" xmlns:c="3" />
```
And so we will call startPrefixMapping in the order of a, b, c. This matces the order that the xmlns appears in the text representation. And I think that's all that matters. How a ContentHandler interprets these mapping and uses them for prefix lookups or converting to text is entirely up to them. Keep in mind they don't have to use NamespaceBindings. Maybe they use a stack of Maps or some complicated data structure. All we are really saying by calling startPrefixMapping is "this is the order we think the xmlns mappings appear in the text representation, you figure out the rest".
And in most cases, order probably doesn't even matter. If they end up creating a NamespaceBinding that is c(b(a)) and do prefix lookups on that, the results will be exactly the same. This is because order doesn't matter in namespace bindings in relation to a single element (*see potential issue at bottom*). For example this is exactly the same as the above XML from a prefix lookup perspective:
```xml
<a xmlns:c="3" xmlns:b="2" xmlns:c="1" />
```
Regardless of the order of those mappings, the a, b, and c prefixes all map to the right uri. A ContentHandler could add those three to a NamespaceBinding (or any data structure of choosing) in any order and prefix lookups would work exactly the same (as long as they took precendece to any outer scope prefixes).
The issue that potentially pops up is if a ContentHandler wants to convert the prefix mappings to XML text, like our DaffodilParseOutputStream thing does. In that case, we might care about the order of things, because it is no longer just about mappings (where order doesn't matter), but is now about outputing XML text (where order does kindof matter because of our tests). So our content handler is careful to outut xmlns text in the same order it recieves the mappings using the new NamespaceBinding logic.
---
I think maybe the only case where order might matter with prefix look ups is if you had something like this:
```xml
<pre:a xmlns:pre="foo" xmlns:pre="bar" />
```
So you have the same prefix that maps to two diferent URIs. In that case, there are two potential namespace bindings depending on order:
```scala
NamespaceBinding("pre", "foo", NamespaceBinding("pre", "bar", null))
NamespaceBinding("pre", "bar", NamespaceBinding("pre", "foo", null))
```
So depending on which NamespaceBinding is used, a prefix lookup of "pre" could return either "foo" or "bar". But it feels to me like redefining a prefix on the same element shouldn't even be allowed--order isn't supposed to matter in prefix mappings, so if prefix redefinition on a single element was allowed, then order would be the only other thing to pick a winner. I would guess this is illegal and isn't even something Daffodil would ever do.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #505:
URL: https://github.com/apache/daffodil/pull/505#issuecomment-828507595
Looks like there's a single test failure:
```
[error] Test org.apache.daffodil.dsom.TestPolymorphicUpwardRelativeExpressions.testPoly2 failed: java.lang.ClassCastException: cannot assign instance of scala.collection.immutable.List$SerializationProxy to field org.apache.daffodil.dsom.DPathCompileInfo.elementCompileInfos of type scala.collection.Seq in instance of org.apache.daffodil.dsom.DPathCompileInfo, took 0.148 sec
[error] at java.io.ObjectStreamClass$FieldReflector.setObjFieldValues(ObjectStreamClass.java:2288)
[error] at java.io.ObjectStreamClass$FieldReflector.checkObjectFieldValueTypes(ObjectStreamClass.java:2251)
...
[error] at java.io.ObjectInputStream.readObject(ObjectInputStream.java:495)
[error] at java.io.ObjectInputStream.readObject(ObjectInputStream.java:453)
[error] at org.apache.daffodil.compiler.Compiler.reload(Compiler.scala:281)
[error] at org.apache.daffodil.util.TestUtils$.saveAndReload(TestUtils.scala:181)
[error] at org.apache.daffodil.util.TestUtils$.runSchemaOnData(TestUtils.scala:194)
[error] at org.apache.daffodil.util.TestUtils$.testString(TestUtils.scala:85)
[error] at org.apache.daffodil.dsom.TestPolymorphicUpwardRelativeExpressions.testPoly2(TestPolymorphicUpwardRelativeExpressions.scala:252)
[error] at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
[error] at jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:78)
[error] at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
[error] at java.lang.reflect.Method.invoke(Method.java:567)
```
Seems there's an issue deserializing this parser with Scala 2.12.13. I can reproduce it on my system without coverage enabled, so likely unrelated to scoverage...
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #505:
URL: https://github.com/apache/daffodil/pull/505#discussion_r623962021
##########
File path: daffodil-cli/src/it/resources/org/apache/daffodil/CLI/output/output9.txt
##########
@@ -1,4 +1,4 @@
-<base14:rabbitHole xmlns:b14="http://b14.com" xmlns:a14="http://a14.com" xmlns:base14="http://baseSchema.com">
+<base14:rabbitHole xmlns:a14="http://a14.com" xmlns:b14="http://b14.com" xmlns:base14="http://baseSchema.com">
Review comment:
Most of our tests either use the TDML runner and so attributes are ignored, or they aren't complex enough to require multiple prefix mappings.
This file is the expected output for one of the few tests where we expect the exact output from the Daffodil CLI. Since we've changed the orer of the namespaces to be consistent and repeatable, regardless of Scala version, this test was affected. So order here doesn't matter, but our test is looking for a specific order because it's looking for exact output.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] olabusayoT commented on a change in pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
olabusayoT commented on a change in pull request #505:
URL: https://github.com/apache/daffodil/pull/505#discussion_r624071841
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/SAXInfosetOutputter.scala
##########
@@ -145,18 +145,11 @@ class SAXInfosetOutputter(xmlReader: DFDL.DaffodilParseXMLReader,
private def doStartPrefixMapping(diElem: DIElement, contentHandler: ContentHandler): Unit = {
val (nsbStart: NamespaceBinding, nsbEnd: NamespaceBinding) = getNsbStartAndEnd(diElem)
var n = nsbStart
- var mappingsList: Seq[(String, String)] = Seq()
while (n.ne(nsbEnd) && n.ne(null) && n.ne(scala.xml.TopScope)) {
val prefix = if (n.prefix == null) "" else n.prefix
val uri = if (n.uri == null) "" else n.uri
- // we generate a list here by prepending so can build the prefixMapping in the
- // same order as it is in minimizedScope when we call startPrefixMapping; we do this because
- // getPrefix in the contentHandler is order dependent
- mappingsList +:= (prefix, uri)
- n = n.parent
- }
- mappingsList.foreach{ case (prefix, uri) =>
contentHandler.startPrefixMapping(prefix, uri)
+ n = n.parent
Review comment:
Hmm let me mentally parse this out, so if we have the minimizedScope as mentioned above, with this new code, we'd call startPrefixMapping in the order below since c is parent of b, which is parent of a
```
contentHandler.startPrefixMapping("a", "foo")
contentHandler.startPrefixMapping("b", "bar")
contentHandler.startPrefixMapping("c", "baz")
```
that will cause currentPrefixMapping to look like:
`NamespaceBinding("c", "baz", NamespaceBinding("b", "bar", NamespaceBinding("a","foo", null)))`
this order is fixed when we convert to activePrefixMapping, which correctly reverses the order to a(b(c)), and our look ups are fine. But to any other content handler what was a(b(c)) in the minimized scope would now have become c(b(a)), no?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] tuxji commented on pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
tuxji commented on pull request #505:
URL: https://github.com/apache/daffodil/pull/505#issuecomment-828473208
Steve, sbt-scoverage has just made a v1.7.0 release that should fix our compilation error and add support for both 2.12.13 and 2.13.5.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on pull request #505:
URL: https://github.com/apache/daffodil/pull/505#issuecomment-829341585
So I think we should close this PR and not do this upgrade. Thoughts?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] olabusayoT commented on a change in pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
olabusayoT commented on a change in pull request #505:
URL: https://github.com/apache/daffodil/pull/505#discussion_r624152186
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/SAXInfosetOutputter.scala
##########
@@ -145,18 +145,11 @@ class SAXInfosetOutputter(xmlReader: DFDL.DaffodilParseXMLReader,
private def doStartPrefixMapping(diElem: DIElement, contentHandler: ContentHandler): Unit = {
val (nsbStart: NamespaceBinding, nsbEnd: NamespaceBinding) = getNsbStartAndEnd(diElem)
var n = nsbStart
- var mappingsList: Seq[(String, String)] = Seq()
while (n.ne(nsbEnd) && n.ne(null) && n.ne(scala.xml.TopScope)) {
val prefix = if (n.prefix == null) "" else n.prefix
val uri = if (n.uri == null) "" else n.uri
- // we generate a list here by prepending so can build the prefixMapping in the
- // same order as it is in minimizedScope when we call startPrefixMapping; we do this because
- // getPrefix in the contentHandler is order dependent
- mappingsList +:= (prefix, uri)
- n = n.parent
- }
- mappingsList.foreach{ case (prefix, uri) =>
contentHandler.startPrefixMapping(prefix, uri)
+ n = n.parent
Review comment:
Ah ok, the last scenario was what I was concerned about. So if that's the case, then this is fine with me!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #505:
URL: https://github.com/apache/daffodil/pull/505#issuecomment-829360951
Actually, I just needed to add a list copy on two members instead of just one. Looking at DPathCompileInfo we actually already do a copy for the typeCalcMap to get around a serialization issue, I'm not sure if it's related:
https://github.com/apache/daffodil/blob/master/daffodil-runtime1/src/main/scala/org/apache/daffodil/dsom/CompiledExpression1.scala#L271-L274
But it does imply there are some issues with serialization in Scala.
I've pushed a fix that shows the new ``map(identity)``'s that fixes this particular issue.
I still don't know what changed in 2.12.12 to make this test start failing now, but I honestly don't understand why this didn't happen in 2.12.11. When I run that minimal reproducable snippet above in scala 2.12.11 then it does fail. So I think this List serialization thing is an issue with all versions of 2.12, and we just happened to get lucky and not hit it in any of our tests.
So I would say the map is an important fix to deal with some serializtion edge cases to probably exist currently.
And if that does fix these issues, which it seems to do, upgrading scala to the 2.12.13 probably includes other fixes we should have.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] olabusayoT commented on a change in pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
olabusayoT commented on a change in pull request #505:
URL: https://github.com/apache/daffodil/pull/505#discussion_r624033344
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/SAXInfosetOutputter.scala
##########
@@ -145,18 +145,11 @@ class SAXInfosetOutputter(xmlReader: DFDL.DaffodilParseXMLReader,
private def doStartPrefixMapping(diElem: DIElement, contentHandler: ContentHandler): Unit = {
val (nsbStart: NamespaceBinding, nsbEnd: NamespaceBinding) = getNsbStartAndEnd(diElem)
var n = nsbStart
- var mappingsList: Seq[(String, String)] = Seq()
while (n.ne(nsbEnd) && n.ne(null) && n.ne(scala.xml.TopScope)) {
val prefix = if (n.prefix == null) "" else n.prefix
val uri = if (n.uri == null) "" else n.uri
- // we generate a list here by prepending so can build the prefixMapping in the
- // same order as it is in minimizedScope when we call startPrefixMapping; we do this because
- // getPrefix in the contentHandler is order dependent
- mappingsList +:= (prefix, uri)
- n = n.parent
- }
- mappingsList.foreach{ case (prefix, uri) =>
contentHandler.startPrefixMapping(prefix, uri)
+ n = n.parent
Review comment:
Relocating my comment here for context:
Correct me if I'm wrong, but these changes might mean our xmlreader cannot be used with another contentHandler and still give the right prefix mappings (when we do lookups), since lookups tend to be order specific, and we can't control how other contentHandlers handle the prefix mappings. This might not be a problem for some users, but maybe we need to bundle the contentHandler with our J/SAPI, if people care about the prefixes?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #505:
URL: https://github.com/apache/daffodil/pull/505#issuecomment-828480653
Cool, I'll update and see how it goes.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on a change in pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #505:
URL: https://github.com/apache/daffodil/pull/505#discussion_r623967359
##########
File path: daffodil-cli/src/it/resources/org/apache/daffodil/CLI/output/output9.txt
##########
@@ -1,4 +1,4 @@
-<base14:rabbitHole xmlns:b14="http://b14.com" xmlns:a14="http://a14.com" xmlns:base14="http://baseSchema.com">
+<base14:rabbitHole xmlns:a14="http://a14.com" xmlns:b14="http://b14.com" xmlns:base14="http://baseSchema.com">
Review comment:
So based on this test being order specific, any given new scala library might change the order we got from Sets & Hash Maps, so could have broken this. Copying the map with identity could traverse it and the re-hash might be different because the order of arrival is different.
So it's very plausible that the serialization change or workarounds changed this.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #505:
URL: https://github.com/apache/daffodil/pull/505#issuecomment-830234182
Which changes are you concerned about?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #505:
URL: https://github.com/apache/daffodil/pull/505#issuecomment-817809118
Current build errors:
```
[error] ## Exception when compiling 3 sources to /home/runner/work/daffodil/daffodil/daffodil-propgen/target/scala-2.12/classes
[error] java.lang.NoSuchMethodError: scala.tools.nsc.Global.reporter()Lscala/tools/nsc/reporters/Reporter;
[error] scoverage.ScoverageInstrumentationComponent$$anon$1.run(plugin.scala:119)
[error] scala.tools.nsc.Global$Run.compileUnitsInternal(Global.scala:1511)
```
The build failures are related to scoverage, which does not support Scala 2.12.13 yet: https://github.com/scoverage/sbt-scoverage/issues/321
Will need to revisit upgrading scala version when that issue is resolved.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #505:
URL: https://github.com/apache/daffodil/pull/505#issuecomment-829447299
I've added a test to assert that we get the expected ClassCastException, so if the serialization issues are ever fixed a test will fail.
The failures with SAX are interesting, and might explain what's going on with the failures. The SAX tests fail because prefix mappings are arriving in the reverse order than they used to. Same prefix mappings, just backwards. No idea why that would happen. I wouldn't think the map(identity) would affect prefix mappings.
Maybe if the ordering of some structure has changed (like maybe iterating over a Map where order shouldn't matter?), that could cause things to also be serialized in a different order, and could avoid hitting the deserialization bug.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] olabusayoT commented on a change in pull request #505: Update scala-library, scala-reflect to 2.12.13
Posted by GitBox <gi...@apache.org>.
olabusayoT commented on a change in pull request #505:
URL: https://github.com/apache/daffodil/pull/505#discussion_r623990195
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DaffodilParseOutputStreamContentHandler.scala
##########
@@ -93,7 +93,12 @@ class DaffodilParseOutputStreamContentHandler(out: OutputStream, pretty: Boolean
override def startPrefixMapping(prefix: String, uri: String): Unit = {
val _prefix = if (prefix == "") null else prefix
- activePrefixMapping = NamespaceBinding(_prefix, uri, activePrefixMapping)
+ // only add this new prefix mapping to the currentElementMapping. The
+ // mappings in this variable will be added to the active mapping later.
+ // This is necessary because we essentially prepend NamespaceBinding's when
Review comment:
's -> s
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org