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

[java][resttemplate] Fix missing javax validation imports with list validation #18332

Merged
merged 5 commits into from
May 22, 2024
Merged

Conversation

horaceli
Copy link
Contributor

@horaceli horaceli commented Apr 8, 2024

Related fix for #17485 which was a regression in 7.2.0 after #17165. That change did not add the necessary imports to the generated API when bean validation was enabled and the schema contained an array that had item level validation.

#17857 only fixed the issue for java native client, this is a similar fix for java resttemplate client.

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh ./bin/configs/*.yaml
    ./bin/utils/export_docs_generators.sh
    
    (For Windows users, please run the script in Git BASH)
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.1.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@nbruno

@cachescrubber @welshm @MelleD @atextor @manedev79 @javisst @borsch @banlevente @Zomzog @martin-mfg

@wing328
Copy link
Member

wing328 commented Apr 8, 2024

thanks for the PR. can you please follow step 3 to update the samples so that the CI can verify the change?

@horaceli
Copy link
Contributor Author

horaceli commented Apr 8, 2024

@wing328

apologies, issue with newline

fixed now, samples should match

@jorgerod
Copy link
Contributor

jorgerod commented Apr 9, 2024

Hi @horaceli @wing328

For webclient it also fails. It would be nice to use this PR to solve both cases. What do you think?

Thank you very much

@horaceli
Copy link
Contributor Author

not sure what is missing, just rebased and reran step 3 and diff is empty

Copy link
Contributor

@martin-mfg martin-mfg left a comment

Choose a reason for hiding this comment

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

Testing the changes to resttemplate with the below input specification, I noticed that the imports are also missing in the generated UserApiTest.java.

Could you please also test the same for apache-httpclient and webclient? And also if these generators have the javax related problem mentioned in my other comment? Thanks!


openapi: 3.0.3
info:
  title: issue-17485
  version: 0.1.0
servers:
  - url: http://api.example.xyz/v1
paths:
  /user:
    get:
      tags:
        - user
      parameters:
        - name: username
          in: path
          required: true
          description: The name of the user
          schema:
            type: array
            items:
              type: string
              pattern: "^[a-zA-Z0-9]$"
      responses:
        "200":
          description: OK

@@ -13,6 +13,11 @@ import java.util.Locale;
import java.util.Map;
import java.util.stream.Collectors;

{{#useBeanValidation}}
import {{javaxPackage}}.validation.constraints.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

The pom.xml generated for resttemplate always includes the jakarta dependency and never the javax dependency. But {{javaxPackage}} used in this line by default refers to javax. So please either change the pom.xml or hard-code jakarta here.

@martin-mfg
Copy link
Contributor

not sure what is missing, just rebased and reran step 3 and diff is empty

I tried re-generating samples and can confirm that it doesn't generate any changes.

@horaceli
Copy link
Contributor Author

horaceli commented Apr 23, 2024

@martin-mfg

I tested the changes against the yaml from #17485 and got the below:

package org.openapitools.client.api;

import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.springframework.web.client.RestClientException;

import java.time.LocalDate;
import java.time.OffsetDateTime;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

/**
 * API tests for UserApi
 */
@Disabled
class UserApiTest {

    private final UserApi api = new UserApi();

    
    /**
     * 
     *
     * 
     *
     * @throws RestClientException
     *          if the Api call fails
     */
    @Test
    void userGetTest() {
        List<String> username = null;

        api.userGet(username);

        // TODO: test validations
    }
    
}

The API tests don't seem to need jakarta validation imports.

Also, addressed the javax/jakarta issue. The javaxPackage placeholder is only relevant for jakarta.annotation-api - jakarta.validation-api will always use jakarta.

@horaceli horaceli marked this pull request as ready for review April 23, 2024 18:12
@horaceli horaceli requested a review from martin-mfg April 24, 2024 16:15
@martin-mfg
Copy link
Contributor

Hi @horaceli,

using the java generator on the input from my previous comment, with library set to "resttemplate" and useBeanValidation set to true, using your latest commit d1633af, I get this output for UserApiTest.java:

[...]
package org.openapitools.client.api;

import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.springframework.web.client.RestClientException;

import java.time.LocalDate;
import java.time.OffsetDateTime;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

/**
 * API tests for UserApi
 */
@Disabled
class UserApiTest {

    private final UserApi api = new UserApi();

    
    /**
     * 
     *
     * 
     *
     * @throws RestClientException
     *          if the Api call fails
     */
    @Test
    void userGetTest() {
        List<@Pattern(regexp = "^[a-zA-Z0-9]$")String> username = null;

        api.userGet(username);

        // TODO: test validations
    }
    
}

This uses @Pattern without importing it. I didn't mention in my previous comment that I used useBeanValidation=true. Sorry for that! Could you please try again with this setting?


The javaxPackage placeholder is only relevant for jakarta.annotation-api - jakarta.validation-api will always use jakarta.

Why?
javax.validation.validation-api also exists, and I think if users set useJakartaEe=false, the generated output should use javax instead of the jakarta equivalent.
I agree that eventually we might want to migrate away from javax completely, but I think this should be a separate PR, and maybe it should even happen in a major version of OpenAPI Generator, not in a minor version (more info).

@horaceli
Copy link
Contributor Author

horaceli commented May 4, 2024

Thanks for the review.

Re the jakarta.validation-api issue, I should have clarified it only applies to generators under modules/openapi-generator/src/main/resources/Java/libraries/. If you look at any pom generated under that directory, e.g. modules/openapi-generator/src/main/resources/Java/libraries/rest-assured/pom.mustache the useJakartaEe flag makes no difference to the validation API, it only affects the jakarta-annotation API. Everywhere in the Java/libraries/* generators, we use jakarta.validation-api:3.0.2 (which uses the jakarta namespace)

This is the same point as you made earlier in #18332 (comment), but I was just applying to all the other generators under Java/libraries

Re the generated API tests, I'll check again and get back to you.

@martin-mfg
Copy link
Contributor

Oh, I wasn't aware the javax problem affects so many different libraries. Good to have it fixed everywhere!

Re the generated API tests, I'll check again and get back to you.

Great.
Just for your information - I won't be available to review any changes at least next week.

@horaceli horaceli requested a review from jimschubert as a code owner May 4, 2024 21:10
@horaceli
Copy link
Contributor Author

horaceli commented May 4, 2024

@martin-mfg

Managed to reproduce the issue for API tests, maybe it was a bad cache, apologies.

I've made the changes now for all the generators I've touched, and added a sample/test with the swagger we've been testing with. Let me know your thoughts.

@@ -0,0 +1,11 @@
generatorName: java
outputDir: samples/client/petstore/java/resttemplate-list-schema-validation
Copy link
Member

@wing328 wing328 May 13, 2024

Choose a reason for hiding this comment

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

can you please add the output folder to .github/workflows/samples-java-client-jdk11.yaml so that the CI will test it moving forward?

@horaceli horaceli requested a review from wing328 May 13, 2024 10:03
@wing328 wing328 added this to the 7.7.0 milestone May 21, 2024
@wing328 wing328 merged commit d0a8726 into OpenAPITools:master May 22, 2024
72 of 74 checks passed
@wing328
Copy link
Member

wing328 commented May 22, 2024

Thanks for the PR, which has been merged.

CI failure fixed via 8860537

@horaceli horaceli deleted the patch-1 branch May 22, 2024 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants