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

Do not serialize null values in path #2105

Conversation

tobilarscheid
Copy link
Contributor

@tobilarscheid tobilarscheid commented Feb 6, 2017

Hi everyone,

when trying to set up swagger with dropwizard, I stumbled over this line. I do understand that it is required by the spec to not serialize null values to the swagger.json file. I do however not really want to globally disable null serialization for my whole application.

I therefore feel that this should directly be annotated at the io.swagger.models.Path class instead of requiring users to change their whole application's config. Please tell me if there is more classes where this change might make sense, I am happy to change them as well.

I ran all tests locally and they passed.

Regards,

Tobi

Ignore null values when serializing io.swagger.models.Path to JSON as null values are not supported in the swagger spec.
@tobilarscheid
Copy link
Contributor Author

This is strange, the build seems to be stalled or something, can someone please check what's wrong?

@fehguy
Copy link
Contributor

fehguy commented Feb 6, 2017

Travis gets mad sometimes, I've restarted it.

@fehguy
Copy link
Contributor

fehguy commented Feb 6, 2017

@tobilarscheid understood--and I agree that what is used for Swagger should not need to apply to the entire application.

I believe the code to serialize the Swagger POJO into JSON lives here. The issue is, the swagger specification does not allow null anywhere in it--that means not serializing null in the Path is only part of the problem. There are many other locations that may have a null value, which will make any consumer of the spec very unhappy if null appears...

Probably the best solution would be to modify the BaseApiListingResource to call the Json.mapper() explicitly--and ensure that that class (which also lives in swagger-core) has the NON_NULL setting in it. Then, you don't care what the global mapper says, we only write with the swagger-specific Json mapper.

Make sense?

@tobilarscheid
Copy link
Contributor Author

Hi @fehguy, I like your idea. The problem is that the serialization does not really happen in the class you linked to - this only generates the jax-rs response which is then serialized into JSON by jackson. I really like that this is the absolute java standard way and I feel we should not change this.
I know that annotating everything with @JsonInclude (like I did for the path) is a lot of work, it is however consistent with how @Json-something annotations are used in the model - they are all over the place already. Also, when looking closer at the 'swagger-models' module and all classes under 'io.swagger.models' it is probably about 10 classes we would actually need to touch - everything else is then dealt with by inheritance.

@tobilarscheid
Copy link
Contributor Author

sorry, accidentally closed

@tobilarscheid tobilarscheid reopened this Feb 6, 2017
@fehguy
Copy link
Contributor

fehguy commented Feb 6, 2017

But @tobilarscheid we can "take over" the serialization process here and return the string rather than letting JAXRS choose what it thinks is best. The problem will become that there are other settings that will break the process--for example you may have an underscore naming rule. Well that's great, but it'll bust the swagger definition, because it is designed as camelCase.

@tobilarscheid
Copy link
Contributor Author

Well, you are right. It may not be most beautiful from the JAX-RS standard point of view, but it sure is the safest solution. If you don't mind I will provide another PR with a customized object mapper used in the BaseApiListingResource. Thanks for your feedback!

@fehguy
Copy link
Contributor

fehguy commented Feb 6, 2017

Sounds good. Consider using the io.swagger.util.Json class in that PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants