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

support json schema generation for @JsonUnwrapped (#271) #706

Merged

Conversation

ciderale
Copy link
Contributor

This pull request relates to #271 which only fixed the "old" schema generation method.
The "new" json schema generation approach (jackson-module-jsonSchema) did not yet handle the JsonUnwrapped annotation.
This patch adds this support by overriding the depositSchemaProperty method of UnwrappingBeanPropertyWriter.
An updated test for verifying the correct behavior is in a separate commit (in jackson-module-jsonSchema).

@cowtowncoder
Copy link
Member

Sounds good. As per my comments on FasterXML/jackson-module-jsonSchema#61 all we need is the CLA and I can merge this in 2.4, as well as push it forward to 2.5 and master.

@cowtowncoder
Copy link
Member

Received CLA, will proceed with merge.

cowtowncoder added a commit that referenced this pull request Feb 17, 2015
support json schema generation for @JsonUnwrapped (#271)
@cowtowncoder cowtowncoder merged commit 7d6f62a into FasterXML:2.4 Feb 17, 2015
cowtowncoder added a commit that referenced this pull request Feb 17, 2015
@cowtowncoder
Copy link
Member

I think I'll want to change JsonFormatVisitorNullWrapper a bit, however. It probably belongs to different package. And name is probably bit misleading as it has nothing to do with nulls.
But it is not used by code in the patch so it is bit hard to say.

@cowtowncoder
Copy link
Member

I'll rename it as JsonFormatVisitorWrapper.Base, to follow pattern used by other similar things in Jackson core.

@ciderale
Copy link
Contributor Author

I agree that this rename makes sense. I thought about that as well, but decided to keep my change minimal (and not exposing anything new with public visibility).

cowtowncoder added a commit that referenced this pull request Feb 17, 2015
@cowtowncoder
Copy link
Member

Yeah I can see both sides; since it's for now only needed for specific use... anyhow, thank you for fixing this. I'll merge other patch next.

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