mbeckerle commented on code in PR #1086:
URL: https://github.com/apache/daffodil/pull/1086#discussion_r1328875223
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/ChoiceAndOtherVariousUnparsers.scala:
##########
@@ -26,14 +28,14 @@ import org.apache.daffodil.runtime1.processors._
import org.apache.daffodil.runtime1.processors.unparsers._
case class ChoiceBranchMap(
- lookupTable: Map[ChoiceBranchEvent, Unparser],
+ lookupTable: java.util.Map[ChoiceBranchEvent, Unparser],
Review Comment:
Suggest centralizing all of this around this type definition
which you then put all the scaladoc/comment documentation on it.
```
type ProperlySerializableMap = java.util.Map
```
Then use ProperlySerializableMap in all the places you currently
java.util.Map explicitly.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/primitives/ChoiceCombinator.scala:
##########
@@ -246,7 +248,19 @@ case class ChoiceCombinator(ch: ChoiceTermBase,
alternatives: Seq[Gram])
}
(parser, isRepresented)
}
- val serializableMap: Map[String, (Parser, Boolean)] =
dispatchBranchKeyMap.map(identity)
+
+ // Scala has a bug where failure to deserialize a scala HashMap when
classpath jars aren't
+ // correct leads to very confusing and unhelpful error messages. This
seems to be related
+ // to the fact that a HashMap serializes to a HashMap$SerializationProxy
which appears to
+ // have a bug in deserialization. The Java HashMap does not have this
issue, so we convert
+ // the scala Map to that. Note that we cannot use just .asJava since
that creates a scala
+ // JavaCollectionWrappers$MapWrapper, which has the same deserialization
issue. Instead,
+ // we pass the Map returned by .asJava to the HashMap constructor, which
copies data from
+ // the MapWrapper and creates a new Java HashMap instance that has no
serialization
+ // issues. We do the same below for the unparse Map. Note that the scala
Vector does not
+ // create a Proxy and so does not have this issue.
Review Comment:
Worth it to mention the DAFFODIL Jira ticket number here, since this Scala
issue may be fixed in the future.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/primitives/ChoiceCombinator.scala:
##########
@@ -246,7 +248,19 @@ case class ChoiceCombinator(ch: ChoiceTermBase,
alternatives: Seq[Gram])
}
(parser, isRepresented)
}
- val serializableMap: Map[String, (Parser, Boolean)] =
dispatchBranchKeyMap.map(identity)
+
+ // Scala has a bug where failure to deserialize a scala HashMap when
classpath jars aren't
+ // correct leads to very confusing and unhelpful error messages. This
seems to be related
+ // to the fact that a HashMap serializes to a HashMap$SerializationProxy
which appears to
+ // have a bug in deserialization. The Java HashMap does not have this
issue, so we convert
+ // the scala Map to that. Note that we cannot use just .asJava since
that creates a scala
+ // JavaCollectionWrappers$MapWrapper, which has the same deserialization
issue. Instead,
+ // we pass the Map returned by .asJava to the HashMap constructor, which
copies data from
+ // the MapWrapper and creates a new Java HashMap instance that has no
serialization
+ // issues. We do the same below for the unparse Map. Note that the scala
Vector does not
+ // create a Proxy and so does not have this issue.
+ val serializableMap: java.util.Map[String, (Parser, Boolean)] =
+ new java.util.HashMap(dispatchBranchKeyMap.asJava)
Review Comment:
I would always use a LinkedHashMap to guarantee repeatable order.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/primitives/ChoiceCombinator.scala:
##########
@@ -319,7 +333,10 @@ case class ChoiceCombinator(ch: ChoiceTermBase,
alternatives: Seq[Gram])
branchForUnparse.get
}
} else {
- val cbm = ChoiceBranchMap(eventUnparserMap, branchForUnparse)
+ // See the parser serializableMap above for why we create this java
HashMap
+ val serializableMap: java.util.Map[ChoiceBranchEvent, Unparser] =
+ new java.util.HashMap(eventUnparserMap.asJava)
Review Comment:
Use LinkedHashMap
--
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]
