stevedlawrence commented on code in PR #1605: URL: https://github.com/apache/daffodil/pull/1605#discussion_r2651052057
########## daffodil-test/src/test/resources/test space/test 1/spaceInNames.tdml: ########## @@ -0,0 +1,51 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + 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. +--> + +<testSuite + suiteName="SpacesInNames" + xmlns="http://www.ibm.com/xmlns/dfdl/testData"; + xmlns:tdml="http://www.ibm.com/xmlns/dfdl/testData";> + + <!-- + Test name: no_namespace_colliding_element_names + Schema: multi_base_05_nons.dfdl.xsd + Root: vagueBase + Purpose: + Verify that schemas located in directories with spaces are handled + correctly, and that duplicate global element names across schemas + with no namespaces result in a Schema Definition Error. + --> + + <tdml:parserTestCase + name="no_namespace_colliding_element_names" Review Comment: I would suggest just calling this `name="spaces_in_names_01"` or something similar, to indicate this is just about verifing spaces in file names works. The fact that there are or aren't namespaces isn't really that important, namespaces just make for a more interesting test. Make sure to update the test name in TestGeneral.scala to match. ########## daffodil-test/src/test/resources/test space/test 1/spaceInNames.tdml: ########## @@ -0,0 +1,51 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + 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. +--> + Review Comment: Since this changes the name of a .tdml file, it means we must also update the test that references the tdml file to use the new name. In this case that is in `TestGeneral.scala`: https://github.com/apache/daffodil/blob/main/daffodil-test/src/test/scala/org/apache/daffodil/section00/general/TestGeneral.scala#L83-L97 So we should also update this test to use spacesInNames.tdml instead of namespaces.tdml and also update that test to reference the `name` of the parser test case. ---- However, that test in `TestGeneral.scala` expects it to not be able to find `namespaces.tdml`. Which it couldn't do before this change. That also probably means it won't be able to find `spacesInNames.tdml` when that is updated. I think this means that the test is just broken--we *should* be able to support spaces in paths and we need tests that verify that works correctly but these tests do not work. I'm pretty sure spaces used to work at some point, so I'm not sure why this test expects failure now. I *think* the issues is in our `Misc` class in the `getResource*` functions: https://github.com/apache/daffodil/blob/main/daffodil-core/src/main/scala/org/apache/daffodil/lib/util/Misc.scala#L103-L129 In both the functions we do `replaceAll("""\s""", "%20")` and then call `Class.getResource` on that path. But that is wrong because `Class.getResource` does not expect `%20` and won't be able to find any resources that contain them. So our `Misc.getResource*` functions are broken with paths that contain spaces. So these replaceAll's probably need to be removed or changed somehow. Note that there are probably other places that expect strings that are escaped URI's so we need to identify and fix those as well. I'm not sure exactly where those would be, but it would probably be revealed in testing. Also note that there are characters other than spaces that need escaping in a URI. Most of those characters are uncommon in files or schema locations, but it does indicate that how we currently handle strings that are sometimes treated as URI's is probably not correct. We don't want to just add more `replaceAll`s, we probably need to think more carefully about how we handle strings and convert them to URI's. Maybe we need something like 'new URL(...).toURI` or something, since I believe URL's support spaces. ---- I think if you update the `TestGeneral.scala` file to replace namespaces.tdml with `spaceInNames.tdml`, that would be sufficient to remove the duplication as mentioned in the original ticket. If you also want to attempt to fix the spaces issues, that could also be done as part of this ticket/PR or we can open another ticket to fix support for spaces. ########## daffodil-test/src/test/resources/test space/test 1/spaceInNames.tdml: ########## @@ -0,0 +1,51 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + 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. +--> + +<testSuite + suiteName="SpacesInNames" + xmlns="http://www.ibm.com/xmlns/dfdl/testData"; + xmlns:tdml="http://www.ibm.com/xmlns/dfdl/testData";> + + <!-- + Test name: no_namespace_colliding_element_names + Schema: multi_base_05_nons.dfdl.xsd + Root: vagueBase + Purpose: + Verify that schemas located in directories with spaces are handled + correctly, and that duplicate global element names across schemas + with no namespaces result in a Schema Definition Error. Review Comment: I'm not sure this tests really wants to care about global element names across schemas. This only cares about spaces. That part of the description can probably be removed. -- 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]
