jdaugherty commented on code in PR #15505: URL: https://github.com/apache/grails-core/pull/15505#discussion_r2935492545
########## grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/MultipartBody.groovy: ########## @@ -0,0 +1,191 @@ +/* + * 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.apache.grails.testing.http.client + +import java.nio.charset.Charset +import java.nio.charset.StandardCharsets + +import groovy.transform.CompileStatic + +/** + * Represents a pre-encoded {@code multipart/form-data} request body. + * <p> + * Use {@code builder()} to create instances. + * + * @since 7.0.9 + */ +@CompileStatic +class MultipartBody { + + /** Multipart boundary token used in body delimiters and the content type header. */ + final String boundary + + /** Full encoded multipart payload bytes. */ + final byte[] bytes + + /** + * @param boundary boundary token for multipart sections + * @param bytes encoded multipart payload + */ + MultipartBody(String boundary, byte[] bytes) { + this.boundary = boundary + this.bytes = bytes + } + + /** + * @return content type header value for this multipart payload + */ + String getContentType() { + "multipart/form-data; boundary=$boundary" + } + + /** + * @return a new multipart body builder + */ + static MultipartBuilder builder() { + new MultipartBuilder() + } + + /** + * Builder used to assemble multipart text and file parts. + */ + @CompileStatic + static class MultipartBuilder { + + private final String boundary = "----jdk-http-${UUID.randomUUID()}" + private final List<Part> parts = [] + private Charset charset = StandardCharsets.UTF_8 + + /** + * Adds a text form field. + * + * @param name form field name + * @param value field value + * @return this builder + */ + MultipartBuilder addPart(String name, String value) { Review Comment: Parts do not necessarily have to be a String value. A good example of this caused me to open this issue recently: https://github.com/cjstehno/ersatz/issues/196 ########## grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/utils/JsonUtils.groovy: ########## @@ -0,0 +1,212 @@ +/* + * 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.apache.grails.testing.http.client.utils + +import java.net.http.HttpResponse + +import groovy.json.JsonOutput +import groovy.json.JsonSlurper +import groovy.transform.CompileStatic + +import org.opentest4j.AssertionFailedError + +/** + * Utility method for handling JSON. + * + * <p>JSON assertion helpers used by HTTP test response expectations.</p> + * + * <p>This utility provides two styles of validation:</p> + * <ul> + * <li><b>Tree equality</b> via {@code verifyJsonTree(java.net.http.HttpResponse, Object)} and + * {@code #verifyJsonTree(java.net.http.HttpResponse, CharSequence)}, where object key order is ignored.</li> + * <li><b>Subset matching</b> via {@code verifyJsonTreeContains(java.net.http.HttpResponse, Object)} and + * {@code verifyJsonTreeContains(java.net.http.HttpResponse, CharSequence)}, where expected map keys + * must exist and expected list elements are matched by index.</li> + * </ul> + * + * <p>Failures include canonicalized JSON representations to make diffs easier to read.</p> + * + * @since 7.0.9 + */ +@CompileStatic +class JsonUtils { + + /** + * Verifies that the response JSON is structurally equal to the expected object tree. + * + * <p>Map keys are compared independent of declaration order.</p> + * + * @param response HTTP response with a JSON body + * @param expected expected JSON-compatible object (for example {@code Map}, {@code List}, scalar) + * @throws AssertionFailedError if the JSON trees differ + */ + static void verifyJsonTree(HttpResponse<?> response, Object expected) { + compareJsonTree(new JsonSlurper().parseText(response.body() as String), expected) Review Comment: This is really a general comment for all of the JsonSlurper usage in this library: Maybe consider a helper to configure the JsonSlurper? There are different types of parsers available per JsonParserType and I've had to switch between them in testing before. ########## grails-test-examples/app1/src/integration-test/groovy/functionaltests/interceptors/InterceptorAdvancedMatchingSpec.groovy: ########## @@ -36,233 +34,201 @@ import grails.testing.mixin.integration.Integration * - Combined matching criteria */ @Integration -class InterceptorAdvancedMatchingSpec extends Specification { - - @Shared - HttpClient client +class InterceptorAdvancedMatchingSpec extends Specification implements HttpClientSupport { def setup() { - client = client ?: HttpClient.create(new URL("http://localhost:$serverPort";)) // Reset interceptor tracking before each test - client.toBlocking().exchange( - HttpRequest.GET('/api/advancedMatching/reset'), - Map - ) - } - - def cleanupSpec() { - client.close() + http('/api/advancedMatching/reset') } // ========== Namespace Matching Tests ========== def "test namespace interceptor matches controller in api namespace"() { when: - def response = client.toBlocking().exchange( - HttpRequest.GET('/api/advancedMatching/index'), - Map - ) + def response = http('/api/advancedMatching/index') then: - response.status.code == 200 - response.body().namespace == 'api' - response.body().interceptors.contains('namespace:api') + response.expectStatus(200) + with(response.json()) { + namespace == 'api' + interceptors.contains('namespace:api') + } } def "test namespace interceptor matches all actions in namespace"() { when: - def response = client.toBlocking().exchange( - HttpRequest.GET('/api/advancedMatching/list'), - Map - ) + def response = http('/api/advancedMatching/list') then: - response.status.code == 200 - response.body().action == 'list' - response.body().interceptors.contains('namespace:api') + response.expectStatus(200) + with(response.json()) { + action == 'list' + interceptors.contains('namespace:api') + } } // ========== HTTP Method Matching Tests ========== def "test method interceptor matches POST requests"() { when: - def response = client.toBlocking().exchange( - HttpRequest.POST('/api/advancedMatching/save', [:]), - Map - ) + def response = httpPostJson('/api/advancedMatching/save', [:]) then: - response.status.code == 200 - response.body().method == 'POST' - response.body().interceptors.contains('method:POST') + response.expectStatus(200) + with(response.json()) { + method == 'POST' + interceptors.contains('method:POST') + } } def "test method interceptor does not match GET requests"() { when: - def response = client.toBlocking().exchange( - HttpRequest.GET('/api/advancedMatching/list'), - Map - ) + def response = http('/api/advancedMatching/list') then: - response.status.code == 200 - !response.body().interceptors.contains('method:POST') + response.expectStatus(200) + !response.json().interceptors.contains('method:POST') } def "test method interceptor matches PUT requests as POST variant"() { when: - def response = client.toBlocking().exchange( - HttpRequest.PUT('/api/advancedMatching/update', [:]), - Map - ) + def response = httpPutJson('/api/advancedMatching/update', [:]) then: - response.status.code == 200 + response.expectStatus(200) // PUT is not POST, so interceptor should not match - !response.body().interceptors.contains('method:POST') + !response.json().interceptors.contains('method:POST') } // ========== Action Exclusion Tests ========== def "test excludes interceptor does not match excluded index action"() { when: - def response = client.toBlocking().exchange( - HttpRequest.GET('/api/advancedMatching/index'), - Map - ) + def response = http('/api/advancedMatching/index') then: - response.status.code == 200 - response.body().action == 'index' - !response.body().interceptors.contains('excludes:index,reset') + response.expectStatus(200) + with(response.json()) { + action == 'index' + !interceptors.contains('excludes:index,reset') + } } def "test excludes interceptor matches non-excluded actions"() { when: - def response = client.toBlocking().exchange( - HttpRequest.GET('/api/advancedMatching/list'), - Map - ) + def response = http('/api/advancedMatching/list') then: - response.status.code == 200 - response.body().action == 'list' - response.body().interceptors.contains('excludes:index,reset') + response.expectStatus(200) + with(response.json()) { + action == 'list' + interceptors.contains('excludes:index,reset') + } } def "test excludes interceptor matches show action"() { when: - def response = client.toBlocking().exchange( - HttpRequest.GET('/api/advancedMatching/show/123'), - Map - ) + def response = http('/api/advancedMatching/show/123') then: - response.status.code == 200 - response.body().action == 'show' - response.body().interceptors.contains('excludes:index,reset') + response.expectStatus(200) + with(response.json()) { + action == 'show' + interceptors.contains('excludes:index,reset') + } } // ========== Multiple Rules (OR) Tests ========== def "test multiple rules interceptor matches show action"() { when: - def response = client.toBlocking().exchange( - HttpRequest.GET('/api/advancedMatching/show/1'), - Map - ) + def response = http('/api/advancedMatching/show/1') then: - response.status.code == 200 - response.body().action == 'show' - response.body().interceptors.contains('multiRule:show|list') + response.expectStatus(200) + with(response.json()) { + action == 'show' + interceptors.contains('multiRule:show|list') + } } def "test multiple rules interceptor matches list action"() { when: - def response = client.toBlocking().exchange( - HttpRequest.GET('/api/advancedMatching/list'), - Map - ) + def response = http('/api/advancedMatching/list') then: - response.status.code == 200 - response.body().action == 'list' - response.body().interceptors.contains('multiRule:show|list') + response.expectStatus(200) + with(response.json()) { + action == 'list' + interceptors.contains('multiRule:show|list') + } } def "test multiple rules interceptor does not match create action"() { when: - def response = client.toBlocking().exchange( - HttpRequest.GET('/api/advancedMatching/create'), - Map - ) + def response = http('/api/advancedMatching/create') then: - response.status.code == 200 - response.body().action == 'create' - !response.body().interceptors.contains('multiRule:show|list') + response.expectStatus(200) + with(response.json()) { + action == 'create' + !interceptors.contains('multiRule:show|list') + } } // ========== Combined Matching Tests ========== def "test multiple interceptors can match same request"() { when: "accessing list action in api namespace" - def response = client.toBlocking().exchange( - HttpRequest.GET('/api/advancedMatching/list'), - Map - ) + def response = http('/api/advancedMatching/list') then: "namespace, excludes, and multiRule interceptors all match" - response.status.code == 200 - response.body().interceptors.contains('namespace:api') - response.body().interceptors.contains('excludes:index,reset') - response.body().interceptors.contains('multiRule:show|list') + response.expectStatus(200) + with(response.json()) { + interceptors.contains('namespace:api') + interceptors.contains('excludes:index,reset') + interceptors.contains('multiRule:show|list') + } } def "test POST to save triggers namespace and method interceptors"() { when: - def response = client.toBlocking().exchange( - HttpRequest.POST('/api/advancedMatching/save', [:]), - Map - ) + def response = httpPostJson('/api/advancedMatching/save', [:]) then: - response.status.code == 200 - response.body().interceptors.contains('namespace:api') - response.body().interceptors.contains('method:POST') - response.body().interceptors.contains('excludes:index,reset') - // save is not in show|list, so multiRule should not match - !response.body().interceptors.contains('multiRule:show|list') + response.expectStatus(200) + with(response.json()) { + interceptors.contains('namespace:api') + interceptors.contains('method:POST') + interceptors.contains('excludes:index,reset') + // save is not in show|list, so multiRule should not match + !interceptors.contains('multiRule:show|list') + } } def "test index action only matches namespace interceptor"() { when: - def response = client.toBlocking().exchange( - HttpRequest.GET('/api/advancedMatching/index'), - Map - ) + def response = http('/api/advancedMatching/index') then: "only namespace interceptor matches (others exclude index)" - response.status.code == 200 - response.body().interceptors.contains('namespace:api') - !response.body().interceptors.contains('excludes:index,reset') - !response.body().interceptors.contains('multiRule:show|list') - !response.body().interceptors.contains('method:POST') + response.expectStatus(200) + with(response.json()) { Review Comment: Is this a spock feature? How do these lines inside the with get asserted? ########## grails-test-examples/views-functional-tests/src/integration-test/groovy/functional/tests/TeamSpec.groovy: ########## @@ -104,15 +68,11 @@ class TeamSpec extends HttpClientSpec { @IgnoreIf({ System.getenv('GITHUB_REF') }) void 'Test HAL rendering'() { when: - HttpRequest request = HttpRequest.GET('/teams/hal/1') - HttpResponse<String> resp = client.toBlocking().exchange(request, String) + def response = http('/teams/hal/1') + def lang = "${System.properties.getProperty('user.language')}_${System.properties.getProperty('user.country')}" Review Comment: Is this actually used? ########## grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/utils/XmlUtils.groovy: ########## @@ -0,0 +1,48 @@ +/* + * 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.apache.grails.testing.http.client.utils + +import groovy.xml.MarkupBuilder + +/** + * Utility methods for handling XML. + * + * @since 7.0.9 + */ +class XmlUtils { + + /** + * Renders XML from the given {@link groovy.xml.MarkupBuilder DSL} closure. + * <p> + * The closure is cloned and executed against a {@link groovy.xml.MarkupBuilder} + * delegate using {@link Closure#DELEGATE_FIRST}. + * + * @param dsl the closure that produces the XML markup + * @return the rendered XML string + */ + static String toXml(@DelegatesTo(strategy = Closure.DELEGATE_FIRST, value = MarkupBuilder) Closure<?> dsl) { + def writer = new StringWriter() + def markupBuilder = new MarkupBuilder(writer) Review Comment: Same as the JsonSlurper, being able to specify configuration for the MarkupBuilder would be useful. i.e. whether attributes are escaped. ########## grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/HttpClientSupportSpec.groovy: ########## @@ -0,0 +1,570 @@ +/* + * 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.apache.grails.testing.http.client + +import java.net.http.HttpClient +import java.net.http.HttpHeaders +import java.net.http.HttpRequest +import java.net.http.HttpResponse +import java.time.Duration +import java.util.concurrent.CompletableFuture +import java.util.concurrent.CountDownLatch +import java.util.concurrent.Executor +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicReference +import java.nio.ByteBuffer +import java.nio.charset.StandardCharsets +import java.util.concurrent.Flow + +import javax.net.ssl.SSLContext +import javax.net.ssl.SSLParameters + +import spock.lang.Specification + +class HttpClientSupportSpec extends Specification { + + void 'getHttpBaseUrl() throws if not set'() { + given: + def testClient = new TestClient() + + when: + testClient.httpBaseUrl + + then: + def e = thrown(IllegalStateException) + e.message.contains('No baseUrl set') + } + + void 'shared client is singleton and reused across threads'() { + given: + def testClient = new TestClient() + def mainThreadClient = testClient.httpClient + + and: + def otherThreadClientRef = new AtomicReference<HttpClient>() + def latch = new CountDownLatch(1) + + when: + Thread.start { + try { + otherThreadClientRef.set(testClient.httpClient) + } finally { + latch.countDown() + } + } + + then: + latch.await(5, TimeUnit.SECONDS) + otherThreadClientRef.get() != null + mainThreadClient.is(otherThreadClientRef.get()) + } + + void 'httpRequestWith uses hard-coded default request timeout'() { + given: + def testClient = new TestClient('http://localhost:8080') Review Comment: One of the lessons learned from Spring was they generally do not advise testing http clients with mock calls. They instead suggest mocking real responses. I've suggested the eratz server for this in the past, and I think it would be good to use here instead. ########## grails-test-examples/micronaut/src/integration-test/groovy/micronaut/MicronautErsatzAdvancedSpec.groovy: ########## @@ -136,37 +124,6 @@ class MicronautErsatzAdvancedSpec extends Specification { ersatz.verify() } - void "ANY method matcher in ersatz matches GET request from Micronaut HttpClient"() { Review Comment: These tests (and others in this file) appear to be testing the behavior of the eratz server - I agree with removing them, but I suspect there is messing test coverage - maybe the AI didn't understand the intent was to call code that then uses a declarative client that calls the mock server? @jamesfredley we may have some test gaps 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]
