Skip to content

Conversation

@jfiala
Copy link
Contributor

@jfiala jfiala commented Jan 6, 2017

PR checklist

  • Read the contribution guildelines.
  • Ran the shell/batch script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates)
  • Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes.

Description of the PR

add beanvalidation annotation support to jaxrs-cxf-cdi

for details see #4091

@wing328 wing328 added this to the v2.2.2 milestone Jan 16, 2017
@wing328
Copy link
Contributor

wing328 commented Jan 19, 2017

@jfiala I pulled this change locally, ran mvn clean package and executed ./bin/jaxrs-cxf-cdi-petstore-server.sh to update the Petstore sample but then I got compilation errors when running mvn test in samples/server/petstore/jaxrs-cxf-cdi/. Here is part of the error:

[ERROR] /Users/williamcheng/Code/swagger-api/swagger-codegen/samples/server/petstore/jaxrs-cxf-cdi/src/gen/java/io/swagger/model/Order.java:[113,10] class, interface, or enum expected
[ERROR] /Users/williamcheng/Code/swagger-api/swagger-codegen/samples/server/petstore/jaxrs-cxf-cdi/src/gen/java/io/swagger/model/Order.java:[115,5] class, interface, or enum expected
[ERROR] /Users/williamcheng/Code/swagger-api/swagger-codegen/samples/server/petstore/jaxrs-cxf-cdi/src/gen/java/io/swagger/model/Order.java:[116,3] class, interface, or enum expected
[ERROR] /Users/williamcheng/Code/swagger-api/swagger-codegen/samples/server/petstore/jaxrs-cxf-cdi/src/gen/java/io/swagger/model/Order.java:[121,10] class, interface, or enum expected
[ERROR] /Users/williamcheng/Code/swagger-api/swagger-codegen/samples/server/petstore/jaxrs-cxf-cdi/src/gen/java/io/swagger/model/Order.java:[123,3] class, interface, or enum expected
[ERROR] /Users/williamcheng/Code/swagger-api/swagger-codegen/samples/server/petstore/jaxrs-cxf-cdi/src/gen/java/io/swagger/model/Order.java:[124,10] class, interface, or enum expected
[ERROR] /Users/williamcheng/Code/swagger-api/swagger-codegen/samples/server/petstore/jaxrs-cxf-cdi/src/gen/java/io/swagger/model/Order.java:[126,3] class, interface, or enum expected
[ERROR] /Users/williamcheng/Code/swagger-api/swagger-codegen/samples/server/petstore/jaxrs-cxf-cdi/src/gen/java/io/swagger/model/Order.java:[130,10] class, interface, or enum expected

Did you get similar errors after updating jaxrs-cxf-cdi Petstore?

wing328 and others added 12 commits January 19, 2017 21:44
* [maven-plugin] allow for ignore file override

The .swagger-codegen-ignore file is beneficial for existing source
directories to provide pattern-based exclusion rules for existing source
to be ignored by swagger codegen. Until now, there's been no utility
other than skipOverwrite to modify the initial generation of code
(either via CLI or maven plugin).

This commit adds support for an ignoreFileOverride option to both the
CLI and the maven plugin.

Example CLI usage:

```
java -jar swagger-codegen.jar generate \
    -i swagger.json -l csharp \
    -o target --ignore-file-override /path/to/ignore-file
```

Example Maven Plugin configuration:

```
    <build>
        <plugins>
            <plugin>
                <groupId>io.swagger</groupId>
                <artifactId>swagger-codegen-maven-plugin</artifactId>
                <version>2.2.2-SNAPSHOT</version>
                <executions>
                    <execution>
                        <goals>
                            <goal>generate</goal>
                        </goals>
                        <configuration>
                            <inputSpec>${project.basedir}/src/main/resources/swagger.yaml</inputSpec>
                            <language>csharp</language>
                            <invokerPackage>io.swagger</invokerPackage>
                            <modelPackage>io.swagger.models</modelPackage>
                            <apiPackage>io.swagger.apis</apiPackage>
                            <ignoreFileOverride>/Users/jim/projects/swagger-codegen/.sample-ignore</ignoreFileOverride>
                            <configOptions>
                            </configOptions>
                        </configuration>
                    </execution>
                </executions>
            </plugin>
        </plugins>
    </build>
```

* [maven-plugin] update new javadocs

* fix bad merge due to missing }
@jfiala
Copy link
Contributor Author

jfiala commented Jan 19, 2017

@wing328 I've had this numerours times, the generated samples are most of the time outdated, pls see #4087 for a suggestion to get this fixed.

The bugs that have already been present (and have nothing to do with the beanvalidation issue I did) are:

  • inner enum templates are outdated, I simply replace them the cxf template I did for jaxrs-cxf
  • missing imports of CXF Multipart, InputStream and wrong reference variable inputStream instead of fileInputStream

I fixed all these for now so they at least compile.

@jfiala
Copy link
Contributor Author

jfiala commented Jan 19, 2017

pls note that the PR here shows too many changes, if I go to my repo and compare I see 58 files changes opposed to 77 files here.
pls let me know if I should re-create the PR to get a clean compare view?
(I had the same issue with PR #4510, there it showed 600 changed files vs. 105 files I actually changed, so I immediately re-created the PR as PR #4600 and now it shows a clean compare view, I had this sometimes after rebasing).

I did the rebase like this:
git fetch upstream
git rebase upstream/master
manually merge conflicts
and then
git push
and for smaller PRs this usually works.

@nickcmaynard
Copy link
Contributor

I think rebasing/recreating this might be a good idea. For example, I'm seeing changes for setIgnoreFilePathOverride, Ruby, jaxrs-spec samples, and PHP, which clearly have nothing to do with bean validation.

Other comments:

  1. beanValidationQueryParams.mustache is extremely similar to beanValidationPathParams.mustache - is it possible to do some refactoring to refine and increase maintainability?
  2. I think splitting the enum changes into their own PR would be a good idea. Typically we should keep PRs small if at all possible.
  3. Please pop my ID into PRs on jaxrs-cxf-cdi so I can help out!

@jfiala jfiala closed this Jan 21, 2017
@jfiala
Copy link
Contributor Author

jfiala commented Jan 21, 2017

@nickcmaynard Thx for your comment.

  1. I also see this, in fact they are all identical and should stay so in the future!
    There are other snippets which have the same issue (e.g. generatedAnnotation.mustache), they have to be maintained for every language separately, although they are identical on purpose and should stay so IMHO.
    @fehguy objected to using mustache lambdas to reuse templates from other languages or centralized templates in a separate directory.
    I think re-using templates using lambdas would make it a lot easier to maintain all the templates and use exceptions only when they are on purpose, not if they happen accidentally because a language is not up to date. A mandatory mustache comment should then be placed inside the exception template to give a resason why this is an exception. I think this would make keeping templates consistent a lot easier, possibly combined with the contract-validation approach of Contract based unit tests #4087.
    A good example of the problem with the current approach is the file generatedAnnotation.mustache, which probably is the same for all Java languages, but currently some have the condition hideGenerationTimestamp but most have not because the languages are not up to date. And there are also different ways of implementation of the hideGenerationTimestamp check, see JavaJaxRS\spec vs. JavaJaxRS. This will become a bigger issue over time, especially as it is not obvious if the templates are not up to date on purpose or because they are not yet updated and tracking these things is tedious (in fact for every change a change must be added to get the change into all other languages).

  2. OK, I'll do so in the future, I hope you're clear with the changes with the reset compare view now. The trouble is, that it is a bit tedious to get things right:
    I do branch X and discover it is out of date. This means I have to checkout master, create a new branch but then also merge the branch Y with the fix branch into the branch X I was originally working on as the branch X would otherwise show the errors in the CI build. I think it would be nice to have a template how to handle this with git correctly.
    However, this problem is less likely to occur if the generated samples are always up to date probably by integrating them into the build as proposed in Contract based unit tests #4087 .

  3. OK, I think maybe could be improved as well, if language maintainers/people interested in a language would be notified automatically instead of having to mention them each time, but I'm not sure what github offers to make this easier...

Pls add review comments to the newly created PR #4615, unfortunately the PR's compare view doesn't get updated after I'm doing a rebase, pls let me know if there is a better way than recreating the PR to update the compare view with the real changes in the branch.

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.

3 participants