Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RESTEASY-3300] Do not append single '&' with empty query parameters #3522

Closed
wants to merge 11 commits into from

Conversation

Dkafetzis
Copy link

Fixed this Jira Issue (https://issues.redhat.com/browse/RESTEASY-3300) and added an integration test for this case

@wildfly-ci
Copy link
Collaborator

Hello, Dkafetzis. I'm waiting for one of the admins to verify this patch with /ok-to-test in a comment.

@jamezp
Copy link
Contributor

jamezp commented Mar 17, 2023

/ok-to-test

Copy link
Contributor

@jamezp jamezp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could actually test this with standard API's rather than internal ones. We could use a ContainerRequestFilter and get the URI from there to check. In some ways this would be the preferred way IMO.

If we want to stick with using internal API's we should have a method like:

private static Client createClient(final String expectedQuery) {
    ResteasyClientBuilder builder = (ResteasyClientBuilder) ClientBuilder.newBuilder();
    ResteasyClient client = builder.httpEngine(new ClientHttpEngine() {
        @Override
        public Response invoke(Invocation request) {
            Assert.assertEquals(expectedQuery, ((ClientInvocation) request).getUri().getQuery());
            return new AbortedResponse(null, new ServerResponse());
        }

        @Override
        public SSLContext getSslContext() {
            return null;
        }

        @Override
        public HostnameVerifier getHostnameVerifier() {
            return null;
        }

        @Override
        public void close() {
        }
    }).build();
}

This would avoid us repeating similar patterns in the test.

This is a little OT for this PR, but I want to remove the usages of ResteascyClientBuilder and the ResteasyClient in the test suite where we can. We should be using standard API's for this as they are available.

@Dkafetzis
Copy link
Author

@jamezp Changes implemented, waiting for review.

@jamezp
Copy link
Contributor

jamezp commented Apr 4, 2023

/retest

@jamezp
Copy link
Contributor

jamezp commented Apr 4, 2023

These Windows test failures are from the test added. Do you have access to a Windows VM @Dkafetzis? If not I have a look locally.

@Dkafetzis
Copy link
Author

Dkafetzis commented Apr 5, 2023

@jamezp should be fine now. Give it another test run

@Dkafetzis Dkafetzis force-pushed the RESTEASY-3300 branch 2 times, most recently from aa43ccf to fd0b021 Compare April 7, 2023 12:01
@Dkafetzis Dkafetzis requested a review from jamezp April 7, 2023 14:20
@Dkafetzis
Copy link
Author

@jamezp All tests passed, I think we can merge this.

Signed-off-by: James R. Perkins <jperkins@redhat.com>
@jamezp
Copy link
Contributor

jamezp commented Apr 20, 2023

replaced by #3570

@jamezp jamezp closed this Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants