You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "mike-mcgann (via GitHub)" <gi...@apache.org> on 2023/05/31 12:08:01 UTC
[GitHub] [daffodil] mike-mcgann opened a new pull request, #992: Remove warnings in serialized parser
mike-mcgann opened a new pull request, #992:
URL: https://github.com/apache/daffodil/pull/992
When a parser is serialized, warnings generated during compilation are also serialized but this behavior is not needed. This change strips out warnings before the contents are serialized.
[DAFFODIL-2803](https://issues.apache.org/jira/browse/DAFFODIL-2803)
--
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
[GitHub] [daffodil] mike-mcgann commented on pull request #992: Remove warnings in serialized parser
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on PR #992:
URL: https://github.com/apache/daffodil/pull/992#issuecomment-1488595799
Closing for now until work can continue
--
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
[GitHub] [daffodil] stevedlawrence merged pull request #992: Remove warnings in serialized parser
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence merged PR #992:
URL: https://github.com/apache/daffodil/pull/992
--
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
[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #992: Remove warnings in serialized parser
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #992:
URL: https://github.com/apache/daffodil/pull/992#discussion_r1144811336
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/DataProcessor.scala:
##########
@@ -295,6 +306,7 @@ class DataProcessor(
variableMap = ssrd.originalVariables, // reset to original variables defined in schema
validationMode =
ValidationMode.Off, // explicitly turn off, so restored processor won't be validating
+ diagnostics = Seq.empty,
Review Comment:
Might be worth adding a comment like the other ones so we document that we are just dropping warnings.
##########
daffodil-test/src/test/resources/org/apache/daffodil/section13/text_number_props/TextNumberProps.tdml:
##########
@@ -3719,7 +3719,7 @@
<tdml:errors>
<tdml:error>Schema Definition Error</tdml:error>
<tdml:error>textStandardExponentRep</tdml:error>
- <tdml:error>cannot</tdml:error>
+ <tdml:error>disallowed</tdml:error>
Review Comment:
Weird, another diagnostics change?
##########
daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_expressions/expressions.tdml:
##########
@@ -7253,8 +7253,7 @@
<tdml:document>1,2,3</tdml:document>
<tdml:errors>
<tdml:error>Schema Definition Error</tdml:error>
- <tdml:error>Statically ambiguous or query-style paths not supported</tdml:error>
- <tdml:error>ex:a</tdml:error>
+ <tdml:error>Path step 'ex:{http://example.com}a' ambiguous</tdml:error>
Review Comment:
This change in diagnostics is odd. Do you know why this happened?
I wonder if the statically ambiguous message still exists as a `tdml:warning`?
Note that the TDMLRunner saves and reloads processors to make sure saving/reload works, so with this change warnings no longer show up when reloading. I wonder if when we reloaded previous to this change if the TDMLRunner incorrectly saw those reloaded warnings as errors? And now that those warnings don't show up on reload things are different and this will actually try to parse, and leads to this path step ambiguous error?
--
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
[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #992: Remove warnings in serialized parser
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #992:
URL: https://github.com/apache/daffodil/pull/992#discussion_r1146183071
##########
daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_expressions/expressions.tdml:
##########
@@ -7253,8 +7253,7 @@
<tdml:document>1,2,3</tdml:document>
<tdml:errors>
<tdml:error>Schema Definition Error</tdml:error>
- <tdml:error>Statically ambiguous or query-style paths not supported</tdml:error>
- <tdml:error>ex:a</tdml:error>
+ <tdml:error>Path step 'ex:{http://example.com}a' ambiguous</tdml:error>
Review Comment:
Yeah, something like that is going on. I spent a little time trying to better understand our TDML runner, because it's still not totally clear to me. I think I have a better idea the core issue (I thin we have a couple related bugs).
In this particular case, the `checkDiagnosticMessages` function is called to verify all expected diagnostics are found:
https://github.com/apache/daffodil/blob/main/daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala#L928-L949
The `diagnostics` parameter contains a combination of diagnostics from schema compilation and from the parse . This function then calls `verifyAllDiagnosticsFound` twice, both times passing in those diagnostics and either the expected errors or expected warnings. The `verifyAllDiagnosticsFound` function iterates over the expected errors or warnings, and makes sure they exist in any of the passed in actual diagnostics.
https://github.com/apache/daffodil/blob/main/daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala#L1784-L1799
But that does no checks to make sure the type of the expected diagnostics matches the type of the actual diagnostics (i.e. error vs warning). So right now, I think there is no difference between `<tdml:errors>` and `<tdml:warnings>` as far as diagnostics checking goes. Note that there is a difference in the logic in other places, for example, if there are expected errors then we may require a failed parse result. So there are other cases where it matters. But this means if there is a parse error, any warnings can be `<tdml:error>` or `<tdml:warning>` and the warning diagnostic will be found.
So that's a bug that needs to be fixed. `verifyAllDiagnosticsFound` should look at the type of each `expectedDiag` and should should accept matches in `actualDiags` of the appropriate error/warning type. Whether or not that is fixed as part of the PR or a new one doesn't really matter to me, it is sort of a different issue. Though it's also probably a fairly small change, so might be worth including in this since it is somewhat related. Note that I think if we fixed this, the `tdml:error` in this test case would need to change to a `tdml:warning` and a new `tdml:error` would be added with this new error message.
But that doesn't explain why the error message changed. So what caused that? You're right that it's because we aren't serializing those warnings, so a save/reload loses those warnings, but the TDMLRuner should capture them and use them for later. When we get a successful `CompileResult`, it is a tuple of compilation warnings and the actual processor, so we do have those warnings even if the processor is saved/reloaded.
I think the issue is where we call `runParseExepctedErrors/Success` here:
https://github.com/apache/daffodil/blob/main/daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala#L976-L1012
In that above chunk, we save the processor from the CompileResult to the `processor` var, but we just ignore whatever diagnostics were part of the `CompileResult`. To get the compile diagnostics for later verification, we end up calling `processor.getDiagnostics` in a number of places. But if we saved/reloaded the processor, the processor no longer has those diagnostics.
So I think the issue is that we should never call `processor.getDiagnostics` (that is an unreliable way to get compile time diags due to this change), but we should instead pass around the diagnostics from the `CompileResult` and use that. And the `getDiagnostics` function should probably just be removed from the `TDMLDFDLProcessor` API--any diagnostics related to compilation should come from the `CompileResult`.
--
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
[GitHub] [daffodil] mike-mcgann commented on pull request #992: Remove warnings in serialized parser
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on PR #992:
URL: https://github.com/apache/daffodil/pull/992#issuecomment-1478012079
I found a case in the test suite where a warning is being converted into an error and I'm not sure if this is desired behavior and how this is happening. Here the test case is looking for an error:
https://github.com/apache/daffodil/blob/main/daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_expressions/expressions.tdml#L7254-L7258
But it is actually a warning here:
https://github.com/apache/daffodil/blob/main/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dsom/CompiledExpression1.scala#L595-L600
I'm not sure how it is getting converted from a warning to an error.
--
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
[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #992: Remove warnings in serialized parser
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #992:
URL: https://github.com/apache/daffodil/pull/992#discussion_r1211616017
##########
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala:
##########
@@ -1033,46 +1036,41 @@ case class ParserTestCase(ptc: NodeSeq, parentArg: DFDLTestSuite)
}
val (parseResult, diagnostics, isError) = {
- if (processor.isError) {
Review Comment:
This is mostly and indentation change, I recommend hiding whiespace when viewing this diff.
--
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
[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #992: Remove warnings in serialized parser
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #992:
URL: https://github.com/apache/daffodil/pull/992#discussion_r1143471339
##########
daffodil-core/src/test/scala/org/apache/daffodil/core/processor/TestSerialization.scala:
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.core.processor
+
+import java.io.File
+import java.io.FileOutputStream
+
+import org.apache.daffodil.core.compiler.Compiler
+
+import org.junit.Assert.assertTrue
+import org.junit.Test
+
+class TestSerialization {
+
+ /**
+ * DAFFODIL-2803
+ *
+ * Check that warnings are not serialized when saving a parser.
+ */
+ @Test def test_stripWarnings() = {
+ val schema =
+ <schema
+ xmlns="http://www.w3.org/2001/XMLSchema"
+ xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/"
+ xmlns:ex="http://example.com"
+ targetNamespace="http://example.com"
+ >
+ <include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <annotation>
+ <!-- The invalid appinfo source generates a warning -->
+ <appinfo source="http://www.ogf.org/dfdl/WRONG">
+ <dfdl:format ref="ex:GeneralFormat"/>
+ </appinfo>
+ </annotation>
+ <element name="root" type="string" dfdl:lengthKind="explicit" dfdl:length="1"/>
+ </schema>
+
+ val factory = Compiler().compileNode(schema)
+ val processor = factory.onPath("/")
+ assertTrue(!processor.getDiagnostics.isEmpty)
+
+ val tmpFile = File.createTempFile("test_stripWarnings", null)
+ tmpFile.deleteOnExit()
+ val output = new FileOutputStream(tmpFile).getChannel()
+ processor.save(output)
+
+ val processor2 = Compiler().reload(tmpFile)
+ assertTrue(processor2.getDiagnostics.isEmpty)
Review Comment:
Instead of creating a temp file and, can we save and reload to/from something in memory? It avoid cruft building up in /tmp/, especially since if you use an sbt console it wont' delete the file until you actually exit. We're really bad about trashing /tmp and I wish we could be better about it.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/DataProcessor.scala:
##########
@@ -284,6 +284,18 @@ class DataProcessor(
// serialize and compress the data processor to the outputstream
val oos = new ObjectOutputStream(new GZIPOutputStream(os))
+ // DAFFODIL-2803
+ // Remove warnings from serialized parser
+ val diagsWithoutWarnings = ssrd.diagnostics.filter(_.isError)
+ val ssrdToSave = new SchemaSetRuntimeData(
+ ssrd.parser,
+ ssrd.unparser,
+ diagsWithoutWarnings,
+ ssrd.elementRuntimeData,
+ ssrd.originalVariables,
+ ssrd.typeCalculators,
+ )
Review Comment:
Since there should only be warnings, and we want to get rid of them, can we just do somethign liek this
```scala
val ssrdToSave = ssrd.copy(diagnostics = Seq.empty)
```
Might need to make `SchemaSetRuntimeData` a case class to get the copy function if it's not already.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/DataProcessor.scala:
##########
@@ -284,6 +284,18 @@ class DataProcessor(
// serialize and compress the data processor to the outputstream
val oos = new ObjectOutputStream(new GZIPOutputStream(os))
+ // DAFFODIL-2803
+ // Remove warnings from serialized parser
+ val diagsWithoutWarnings = ssrd.diagnostics.filter(_.isError)
Review Comment:
There should *only* be warnings. If there are any errors then we shouldn't even allow saving, and probably want to throw a usage error.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/DataProcessor.scala:
##########
@@ -292,6 +304,7 @@ class DataProcessor(
// them back to their original values.
//
val dpToSave = this.copy(
+ ssrd = ssrdToSave,
Review Comment:
`SchemaSetRuntimeData` has this comment about the `diagnostics` member:
```scala
/*
* Memory of the compiler's warnings. If we save a processor, it's useful to be able
* to have these warnings.
*/
```
This comment is now wrong and should probably be removed. Turns out it's not actually useful and just leads to too much verbose messages.
In fact, I wonder if SSRD shouldn't even contain diagnostics and they should be somewhere else? No other RuntimeData objects contain diagnostics. Maybe the diags want to be stored on the `DataProcessor`, and those are removed when we save just like `variableMap` and `validationMode`? I'm not sure if `diagnostics` were moved from somewhere else to the SSRD so they could be serialized or if they were always there, but the `DataProcessor` seems like a reasonable place to store them. Also avoids the need to copy the SSRD since we are already copying the DataProcessor when we save. Simplifies things just a little 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.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mike-mcgann commented on a diff in pull request #992: Remove warnings in serialized parser
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #992:
URL: https://github.com/apache/daffodil/pull/992#discussion_r1143904635
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/DataProcessor.scala:
##########
@@ -292,6 +304,7 @@ class DataProcessor(
// them back to their original values.
//
val dpToSave = this.copy(
+ ssrd = ssrdToSave,
Review Comment:
I removed the diagnostics from the SSRD and moved them up to the Processor.
--
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
[GitHub] [daffodil] tuxji commented on a diff in pull request #992: Remove warnings in serialized parser
Posted by "tuxji (via GitHub)" <gi...@apache.org>.
tuxji commented on code in PR #992:
URL: https://github.com/apache/daffodil/pull/992#discussion_r1145044999
##########
daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_expressions/expressions.tdml:
##########
@@ -7253,8 +7253,7 @@
<tdml:document>1,2,3</tdml:document>
<tdml:errors>
<tdml:error>Schema Definition Error</tdml:error>
- <tdml:error>Statically ambiguous or query-style paths not supported</tdml:error>
- <tdml:error>ex:a</tdml:error>
+ <tdml:error>Path step 'ex:{http://example.com}a' ambiguous</tdml:error>
Review Comment:
Steve's hypothesis sounds worth checking to me. Mike, I wonder if running "daffodil parse ..." by hand on the given TDML test case(s) with both old and new daffodil builds would show any changes in the diagnostics. If you don't see any change, then the saving and reloading of the parser must indeed be causing the change in the diagnostics the TDML runner sees.
--
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
[GitHub] [daffodil] tuxji commented on a diff in pull request #992: Remove warnings in serialized parser
Posted by "tuxji (via GitHub)" <gi...@apache.org>.
tuxji commented on code in PR #992:
URL: https://github.com/apache/daffodil/pull/992#discussion_r1148440152
##########
daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_expressions/expressions.tdml:
##########
@@ -7253,8 +7253,7 @@
<tdml:document>1,2,3</tdml:document>
<tdml:errors>
<tdml:error>Schema Definition Error</tdml:error>
- <tdml:error>Statically ambiguous or query-style paths not supported</tdml:error>
- <tdml:error>ex:a</tdml:error>
+ <tdml:error>Path step 'ex:{http://example.com}a' ambiguous</tdml:error>
Review Comment:
I agree with @stevedlawrence's suggestion to remove `processor.getDiagnostics` calls and get diagnostics from the `CompileResult`, `TDMLParseResult`, and `TDMLUnparseResult` instead. When I implemented the `TDMLDFDLProcessor` API in `DaffodilCTDMLDFDLProcessor`, I already made its `isError` and `getDiagnostics` no-ops:
```scala
// We return errors and diagnostics in TDMLParseResult and TDMLUnparseResult
override def isError: Boolean = false
override def getDiagnostics: Seq[Diagnostic] = Seq.empty
```
These bigger changes may break some tests, but it should be worth fixing some tests.
--
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
[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #992: Remove warnings in serialized parser
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #992:
URL: https://github.com/apache/daffodil/pull/992#discussion_r1211616695
##########
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala:
##########
@@ -1639,59 +1634,53 @@ case class UnparserTestCase(ptc: NodeSeq, parentArg: DFDLTestSuite)
case e: Exception => throw TDMLException(e, implString)
}
- val diagnostics = {
Review Comment:
This is mostly an indentation change, I recommend hiding whitespace when viewing this diff.
--
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
[GitHub] [daffodil] mike-mcgann commented on pull request #992: Remove warnings in serialized parser
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on PR #992:
URL: https://github.com/apache/daffodil/pull/992#issuecomment-1478473961
I'm not sure why it was looking like that it was originally expecting some errors from warnings but those warnings are now warnings and there are similar error messages but not the exact same text.
--
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
[GitHub] [daffodil] tuxji commented on a diff in pull request #992: Remove warnings in serialized parser
Posted by "tuxji (via GitHub)" <gi...@apache.org>.
tuxji commented on code in PR #992:
URL: https://github.com/apache/daffodil/pull/992#discussion_r1145044999
##########
daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_expressions/expressions.tdml:
##########
@@ -7253,8 +7253,7 @@
<tdml:document>1,2,3</tdml:document>
<tdml:errors>
<tdml:error>Schema Definition Error</tdml:error>
- <tdml:error>Statically ambiguous or query-style paths not supported</tdml:error>
- <tdml:error>ex:a</tdml:error>
+ <tdml:error>Path step 'ex:{http://example.com}a' ambiguous</tdml:error>
Review Comment:
Steve's hypothesis sounds worth checking to me. Mike, I wonder if running "daffodil parse ..." by hand on the given TDML test case(s) with both old and new daffodil builds would show any changes in the diagnostics. If you don't see any change, then the saving and reloading of the parser must indeed be causing the change in the diagnostics the TDML runner sees. Note that you also can save and reload the parser on the command line in order to observe any changes to the diagnostics.
--
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
[GitHub] [daffodil] mike-mcgann commented on a diff in pull request #992: Remove warnings in serialized parser
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #992:
URL: https://github.com/apache/daffodil/pull/992#discussion_r1143903994
##########
daffodil-core/src/test/scala/org/apache/daffodil/core/processor/TestSerialization.scala:
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.core.processor
+
+import java.io.File
+import java.io.FileOutputStream
+
+import org.apache.daffodil.core.compiler.Compiler
+
+import org.junit.Assert.assertTrue
+import org.junit.Test
+
+class TestSerialization {
+
+ /**
+ * DAFFODIL-2803
+ *
+ * Check that warnings are not serialized when saving a parser.
+ */
+ @Test def test_stripWarnings() = {
+ val schema =
+ <schema
+ xmlns="http://www.w3.org/2001/XMLSchema"
+ xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/"
+ xmlns:ex="http://example.com"
+ targetNamespace="http://example.com"
+ >
+ <include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <annotation>
+ <!-- The invalid appinfo source generates a warning -->
+ <appinfo source="http://www.ogf.org/dfdl/WRONG">
+ <dfdl:format ref="ex:GeneralFormat"/>
+ </appinfo>
+ </annotation>
+ <element name="root" type="string" dfdl:lengthKind="explicit" dfdl:length="1"/>
+ </schema>
+
+ val factory = Compiler().compileNode(schema)
+ val processor = factory.onPath("/")
+ assertTrue(!processor.getDiagnostics.isEmpty)
+
+ val tmpFile = File.createTempFile("test_stripWarnings", null)
+ tmpFile.deleteOnExit()
+ val output = new FileOutputStream(tmpFile).getChannel()
+ processor.save(output)
+
+ val processor2 = Compiler().reload(tmpFile)
+ assertTrue(processor2.getDiagnostics.isEmpty)
Review Comment:
Updated to save/load to memory
--
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
[GitHub] [daffodil] stevedlawrence commented on pull request #992: Remove warnings in serialized parser
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on PR #992:
URL: https://github.com/apache/daffodil/pull/992#issuecomment-1478022312
> I found a case in the test suite where a warning is being converted into an error and I'm not sure if this is desired behavior and how this is happening. Here the test case is looking for an error:
Thats odd. I have no idea why that would be happening, but it feels incorrect. It's not even clear to me why that's a warning. I didn't think query style expressions were supported at all...
--
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
[GitHub] [daffodil] mike-mcgann commented on a diff in pull request #992: Remove warnings in serialized parser
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #992:
URL: https://github.com/apache/daffodil/pull/992#discussion_r1145255037
##########
daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_expressions/expressions.tdml:
##########
@@ -7253,8 +7253,7 @@
<tdml:document>1,2,3</tdml:document>
<tdml:errors>
<tdml:error>Schema Definition Error</tdml:error>
- <tdml:error>Statically ambiguous or query-style paths not supported</tdml:error>
- <tdml:error>ex:a</tdml:error>
+ <tdml:error>Path step 'ex:{http://example.com}a' ambiguous</tdml:error>
Review Comment:
I think it is this line here:
https://github.com/apache/daffodil/blob/main/daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala#L978
And I think what is happening is that compile-time warnings are getting converted to errors but runtime warnings are 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
[GitHub] [daffodil] stevedlawrence commented on pull request #992: Remove warnings in serialized parser
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on PR #992:
URL: https://github.com/apache/daffodil/pull/992#issuecomment-1570111637
@mike-mcgann is busy on other stuff, so I took over this and fixed up the last remaining comments. This PR is ready for a rereview.
--
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
[GitHub] [daffodil] mike-mcgann closed pull request #992: Remove warnings in serialized parser
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann closed pull request #992: Remove warnings in serialized parser
URL: https://github.com/apache/daffodil/pull/992
--
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
[GitHub] [daffodil] mike-mcgann commented on a diff in pull request #992: Remove warnings in serialized parser
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #992:
URL: https://github.com/apache/daffodil/pull/992#discussion_r1143534607
##########
daffodil-core/src/test/scala/org/apache/daffodil/core/processor/TestSerialization.scala:
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.core.processor
+
+import java.io.File
+import java.io.FileOutputStream
+
+import org.apache.daffodil.core.compiler.Compiler
+
+import org.junit.Assert.assertTrue
+import org.junit.Test
+
+class TestSerialization {
+
+ /**
+ * DAFFODIL-2803
+ *
+ * Check that warnings are not serialized when saving a parser.
+ */
+ @Test def test_stripWarnings() = {
+ val schema =
+ <schema
+ xmlns="http://www.w3.org/2001/XMLSchema"
+ xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/"
+ xmlns:ex="http://example.com"
+ targetNamespace="http://example.com"
+ >
+ <include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <annotation>
+ <!-- The invalid appinfo source generates a warning -->
+ <appinfo source="http://www.ogf.org/dfdl/WRONG">
+ <dfdl:format ref="ex:GeneralFormat"/>
+ </appinfo>
+ </annotation>
+ <element name="root" type="string" dfdl:lengthKind="explicit" dfdl:length="1"/>
+ </schema>
+
+ val factory = Compiler().compileNode(schema)
+ val processor = factory.onPath("/")
+ assertTrue(!processor.getDiagnostics.isEmpty)
+
+ val tmpFile = File.createTempFile("test_stripWarnings", null)
+ tmpFile.deleteOnExit()
+ val output = new FileOutputStream(tmpFile).getChannel()
+ processor.save(output)
+
+ val processor2 = Compiler().reload(tmpFile)
+ assertTrue(processor2.getDiagnostics.isEmpty)
Review Comment:
Yes, I was having trouble finding out how to do this with channels but I found the solution elsewhere in the code.
--
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
[GitHub] [daffodil] mike-mcgann commented on a diff in pull request #992: Remove warnings in serialized parser
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #992:
URL: https://github.com/apache/daffodil/pull/992#discussion_r1145297199
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/DataProcessor.scala:
##########
@@ -295,6 +306,7 @@ class DataProcessor(
variableMap = ssrd.originalVariables, // reset to original variables defined in schema
validationMode =
ValidationMode.Off, // explicitly turn off, so restored processor won't be validating
+ diagnostics = Seq.empty,
Review Comment:
Updated
--
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
[GitHub] [daffodil] mike-mcgann commented on a diff in pull request #992: Remove warnings in serialized parser
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #992:
URL: https://github.com/apache/daffodil/pull/992#discussion_r1145258384
##########
daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_expressions/expressions.tdml:
##########
@@ -7253,8 +7253,7 @@
<tdml:document>1,2,3</tdml:document>
<tdml:errors>
<tdml:error>Schema Definition Error</tdml:error>
- <tdml:error>Statically ambiguous or query-style paths not supported</tdml:error>
- <tdml:error>ex:a</tdml:error>
+ <tdml:error>Path step 'ex:{http://example.com}a' ambiguous</tdml:error>
Review Comment:
And I think the "change" is that there was a compile-time warning but also a runtime error for the same condition. So the tests were originally checking for the warning (which got converted to an error), but now that those are not serialized, we are now seeing the associated runtime error.
--
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
[GitHub] [daffodil] mike-mcgann commented on a diff in pull request #992: Remove warnings in serialized parser
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #992:
URL: https://github.com/apache/daffodil/pull/992#discussion_r1151036684
##########
daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_expressions/expressions.tdml:
##########
@@ -7253,8 +7253,7 @@
<tdml:document>1,2,3</tdml:document>
<tdml:errors>
<tdml:error>Schema Definition Error</tdml:error>
- <tdml:error>Statically ambiguous or query-style paths not supported</tdml:error>
- <tdml:error>ex:a</tdml:error>
+ <tdml:error>Path step 'ex:{http://example.com}a' ambiguous</tdml:error>
Review Comment:
I've updated the runner to now check to see if warnings match up to warnings and if errors match up to errors. Quite a few existing tests had to change as they were relying on the previous behavior. I'm looking into removing the `getDiagnostics` and `isError` definitions next.
--
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