Copilot commented on code in PR #16637: URL: https://github.com/apache/grails-core/pull/16637#discussion_r4264431661
########## grails-web-common/src/test/groovy/org/grails/web/util/BoundedCharsAsEncodedBytesCounterSpec.groovy: ########## @@ -0,0 +1,161 @@ +/* + * 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 + * + * https://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.grails.web.util + +import java.nio.charset.UnsupportedCharsetException + +import spock.lang.Specification + +class BoundedCharsAsEncodedBytesCounterSpec extends Specification { + + def "counts encoded bytes through string and character updates"() { + given: + def counter = new BoundedCharsAsEncodedBytesCounter(32, encoding) + counter.getCountingWriter() + + when: + counter.update('é') + counter.update('xöx'.toCharArray(), 1, 1) + counter.update('a'.toCharArray()) + counter.update('') + counter.update(new char[0]) + + then: + counter.size() == expected + + where: + encoding | expected + 'UTF-8' | 5 + 'ISO-8859-1' | 3 + } + + def "writer is initialized lazily and reused with configured capacity and encoding"() { + given: + def counter = new BoundedCharsAsEncodedBytesCounter() + counter.setCapacity(16) + counter.setEncoding('UTF-8') + + expect: + counter.capacity == 16 + counter.encoding == 'UTF-8' + !counter.writerReferenced + + when: + def writer = counter.getCountingWriter() + writer.write('xéx'.toCharArray(), 1, 1) + writer.write(65) + writer.write('xbx'.toCharArray(), 1, 1) + writer.append(new StringBuilder('more')) + writer.flush() + writer.close() + + then: + counter.writerReferenced + counter.getCountingWriter().is(writer) + counter.size() == 8 Review Comment: This test asserts the same writer instance is returned even after `writer.close()`. Reusing a closed `Writer` is generally not a safe/expected contract; if the production code later changes to recreate the writer after close (still correct behavior), this test will fail. Consider avoiding `close()` here (use `flush()` only), or explicitly asserting the intended contract (either 'close is a no-op and reuse is supported' or 'close forces a new writer'). ########## grails-web-common/src/test/groovy/org/grails/web/util/WebUtilsSpec.groovy: ########## @@ -26,18 +33,236 @@ import spock.lang.Specification */ class WebUtilsSpec extends Specification { - @Issue("https://github.com/apache/grails-core/issues/10545";) - def testToQueryString() { - given: - Map params = ["name":"sudhir-nimavat", "address.zip":"12345"] + @Issue("https://github.com/apache/grails-core/issues/10545";) + def testToQueryString() { + given: + Map params = ["name":"sudhir-nimavat", "address.zip":"12345"] - when: - String result = WebUtils.toQueryString(params) + when: + String result = WebUtils.toQueryString(params) - then: - result.startsWith("?") - def tokens = result[1..-1].split('&') - tokens.find({ it == "name=sudhir-nimavat"}) != null - tokens.find({ it == "address.zip=12345"}) != null - } + then: + result.startsWith("?") + def tokens = result[1..-1].split('&') + tokens.find({ it == "name=sudhir-nimavat"}) != null + tokens.find({ it == "address.zip=12345"}) != null + } + + def "decodes query strings and collects repeated parameters in order"() { + expect: + WebUtils.fromQueryString('?name=caf%C3%A9&tag=one&tag=two&tag=three&empty=&ignored&first+name=A+B') == [ + name: 'café', tag: ['one', 'two', 'three'], empty: '', 'first name': 'A B' + ] + WebUtils.fromQueryString('') == [:] + WebUtils.fromQueryString('name=value') == [name: 'value'] + } + + def "rejects malformed percent escapes in query strings"() { + when: + WebUtils.fromQueryString('name=%ZZ') + + then: + thrown(IllegalArgumentException) + } + + def "adds controller prefixes only to relative view names"() { + expect: + WebUtils.addViewPrefix(viewName, controllerName) == expected + + where: + viewName | controllerName | expected + 'show' | 'book' | '/book/show' + 'show' | null | '/show' + '/shared/show' | 'book' | '/shared/show' + 'redirect:/login' | 'book' | 'redirect:/login' + 'forward:/internal' | 'book' | 'forward:/internal' + } + + def "resolves a controller view using the current default locale without a web request"() { + given: + def request = new MockHttpServletRequest() + def resolver = Mock(ViewResolver) + def view = Stub(View) + + when: + def resolved = WebUtils.resolveView(request, 'show', 'book', resolver) + + then: + 1 * resolver.resolveViewName('/book/show', Locale.getDefault()) >> view + resolved.is(view) + } Review Comment: This test depends on `Locale.getDefault()` at assertion time, which can make it flaky if other tests mutate the default locale. Consider setting a known default locale for the duration of this test (and restoring the previous locale in `cleanup`) or asserting against `request.locale`/a fixed locale if `resolveView` is expected to use one deterministically. ########## grails-web-common/src/test/groovy/org/grails/web/xml/StreamingMarkupWriterSpec.groovy: ########## @@ -0,0 +1,76 @@ +/* + * 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 + * + * https://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.grails.web.xml + +import spock.lang.Specification + +class StreamingMarkupWriterSpec extends Specification { + + def "escapes double quotes only in attributes while preserving XML escaping"() { + given: + def target = new StringWriter() + def writer = new StreamingMarkupWriter(target) + writer.setWritingAttribute(attribute) + + when: + writer.escaped().write('"<&>') + + then: + target.toString() == expected + + where: + attribute | expected + true | '"<&>' + false | '"<&>' + } + + def "switches from attributes to text and permits unescaped markup"() { + given: + def target = new StringWriter() + def writer = new StreamingMarkupWriter(target, 'UTF-8') + + when: + writer.unescaped().write('<node title="') + writer.setWritingAttribute(true) + writer.write(34) + writer.setWritingAttribute(false) + writer.unescaped().write('">') + writer.escaped().write('"café"') + writer.unescaped().write('</node>') + writer.flush() + + then: + target.toString() == '<node title=""">"café"</node>' + } + + def "propagates failures when escaping an attribute quote"() { + given: + def target = Mock(Writer) + def writer = new StreamingMarkupWriter(target) + writer.setWritingAttribute(true) + + when: + writer.write(34) + + then: + 1 * target.write('"') >> { throw new IOException('output failed') } + def error = thrown(IOException) + error.message == 'output failed' Review Comment: The interaction `1 * target.write('"')` is brittle because it depends on the exact `Writer.write(...)` overload used internally (e.g., `write(String)` vs `write(int)` vs `write(char[],...)`). To make the test resilient to refactors that preserve behavior, match more flexibly (e.g., accept any write call whose content equals `"`, regardless of overload), while still throwing the same `IOException`. ########## grails-web-common/src/test/groovy/org/grails/web/xml/PrettyPrintXMLStreamWriterSpec.groovy: ########## @@ -0,0 +1,112 @@ +/* + * 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 + * + * https://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.grails.web.xml + +import spock.lang.Specification + +class PrettyPrintXMLStreamWriterSpec extends Specification { + + def "pretty prints a document with nested elements attributes escaped text and empty elements"() { + given: + def target = new StringWriter() + def writer = new PrettyPrintXMLStreamWriter(new StreamingMarkupWriter(target, 'UTF-8')) + def newline = PrettyPrintXMLStreamWriter.NEWLINE + + when: + writer.startDocument('UTF-8', '1.0') + .startNode('root').attribute('id', 'a"&b') + .startNode('child').characters('a < b & c > d').end() + .startNode('empty').end() + .end() + + then: + target.toString() == [ + '<?xml version="1.0" encoding="UTF-8"?>', + '<root id="a"&b">', + ' <child>', + ' a < b & c > d', + ' </child>', + ' <empty />', + '</root>', + '' + ].join(newline) + } + + def "uses custom indentation at each nested level"() { + given: + def target = new StringWriter() + def writer = new PrettyPrintXMLStreamWriter(new StreamingMarkupWriter(target), '....') + + when: + writer.startNode('outer').startNode('middle').startNode('inner').end().end().end() + + then: + target.toString() == [ + '<outer>', + '....<middle>', + '........<inner />', + '....</middle>', + '</outer>', + '' + ].join(PrettyPrintXMLStreamWriter.NEWLINE) + } + + def "rejects attributes outside an open start tag"() { + given: + def writer = new PrettyPrintXMLStreamWriter(new StreamingMarkupWriter(new StringWriter())) + if (afterText) { + writer.startNode('root').characters('body') + } + + when: + writer.attribute('id', 'invalid') + + then: + thrown(IllegalStateException) + + where: + afterText << [false, true] + } + + def "rejects a document declaration after an element has started"() { + given: + def writer = new PrettyPrintXMLStreamWriter(new StreamingMarkupWriter(new StringWriter())) + writer.startNode('root') + + when: + writer.startDocument('UTF-8', '1.0') + + then: + thrown(IllegalStateException) + } + + def "propagates output failures"() { + given: + def target = Mock(Writer) + def writer = new PrettyPrintXMLStreamWriter(new StreamingMarkupWriter(target)) + + when: + writer.startNode('root') + + then: + 1 * target.write(60) >> { throw new IOException('output failed') } + def error = thrown(IOException) + error.message == 'output failed' Review Comment: This mock expectation ties the test to a specific low-level write pattern (`write(60)` for '<'). If the implementation switches to `write('<root>')`, buffered writes, or `write(String)`, the behavior is still correct but the test will fail. Prefer matching the first write more semantically (e.g., any write that contains `<` or starts the tag) and still raise the `IOException`. -- 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]
