Copilot commented on code in PR #15958: URL: https://github.com/apache/grails-core/pull/15958#discussion_r3560650534
########## grails-web-common/src/main/groovy/org/grails/web/json/GroovyJsonFacade.java: ########## @@ -0,0 +1,75 @@ +/* + * 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.json; + +import java.util.Collection; +import java.util.Map; + +import groovy.json.JsonOutput; +import groovy.json.JsonSlurper; + +/** + * Facade for moving JSON parsing and rendering toward groovy-json while preserving Grails JSONElement types. + * + * @since 8.1 + */ +public final class GroovyJsonFacade { + + private GroovyJsonFacade() { + } + + public static JSONElement parse(String json) { + Object value = new JsonSlurper().parseText(json); + Object converted = toGrailsJson(value); + if (converted instanceof JSONElement) { + return (JSONElement) converted; + } + throw new JSONException("JSON text must describe an object or array"); + } Review Comment: GroovyJsonFacade.parse currently leaks groovy-json parsing exceptions (e.g. groovy.json.JsonException / IllegalArgumentException) instead of consistently throwing org.grails.web.json.JSONException like the existing JSON facade APIs. That makes the new facade harder to adopt as a drop-in replacement for the JSONTokener-based path and breaks callers that only catch JSONException. ########## grails-web-common/src/test/groovy/org/grails/web/json/GroovyJsonFacadeSpec.groovy: ########## @@ -0,0 +1,44 @@ +/* + * 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.json + +import spock.lang.Specification + +class GroovyJsonFacadeSpec extends Specification { + + void 'parse returns existing JSONObject facade backed by groovy-json parsing'() { + when: + JSONElement element = GroovyJsonFacade.parse('{"name":"Grails","versions":[8,9]}') + + then: + element instanceof JSONObject + element.get('name') == 'Grails' + element.getJSONArray('versions').getInt(0) == 8 + } + + void 'toJson renders existing JSONObject facade through groovy-json'() { + given: + JSONObject object = new JSONObject() + object.put('name', 'Grails') + object.put('active', true) + + expect: + GroovyJsonFacade.toJson(object) == '{"name":"Grails","active":true}' Review Comment: This assertion relies on deterministic JSON key ordering, but JSONObject is backed by a HashMap (iteration order is unspecified). JsonOutput serializes Maps using iteration order, so this test can be flaky across JDKs/runs. ########## grails-web-common/src/test/groovy/org/grails/web/json/GroovyJsonFacadeSpec.groovy: ########## @@ -0,0 +1,44 @@ +/* + * 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.json + +import spock.lang.Specification + +class GroovyJsonFacadeSpec extends Specification { + + void 'parse returns existing JSONObject facade backed by groovy-json parsing'() { + when: + JSONElement element = GroovyJsonFacade.parse('{"name":"Grails","versions":[8,9]}') + + then: + element instanceof JSONObject + element.get('name') == 'Grails' + element.getJSONArray('versions').getInt(0) == 8 + } + + void 'toJson renders existing JSONObject facade through groovy-json'() { Review Comment: The facade introduces distinct behaviors that aren't covered yet (custom guideline: every code touch must update/extend tests). In particular, parsing invalid JSON and rejecting scalar roots are important edge cases for callers and should be verified to avoid leaking groovy-json exceptions or regressing the error contract. ########## grails-web-common/build.gradle: ########## @@ -49,6 +49,7 @@ dependencies { } api 'org.apache.groovy:groovy' + api 'org.apache.groovy:groovy-json' api 'org.apache.groovy:groovy-templates' Review Comment: groovy-json is only used internally by GroovyJsonFacade and is not part of this module's public API surface (no public signatures reference groovy.json.* types). Declaring it as api unnecessarily exposes it transitively to downstream consumers; implementation is sufficient here. -- 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]
