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.

Sign up for GitHub

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

Jump to bottom

Deprecate classes in package com.fasterxml.jackson.databind.jsonschema #3745

Merged
merged 3 commits into from Jan 29, 2023
Merged

Deprecate classes in package com.fasterxml.jackson.databind.jsonschema #3745

merged 3 commits into from Jan 29, 2023

Conversation

luozhenyu
Copy link
Contributor

The package com.fasterxml.jackson.databind.jsonschema has moved to an external module JSON Schema generator module and all classes in this package should be marked with @Deprecated.

This PR deprecated classes and methods in package jsonschema. Programmers implementing interface SchemaAware or override method public JsonNode getSchema(SerializerProvider provider, Type typeHint) will get a deprecation warning.

@luozhenyu luozhenyu marked this pull request as ready for review January 20, 2023 09:53
cowtowncoder
cowtowncoder reviewed Jan 20, 2023
View reviewed changes
src/main/java/com/fasterxml/jackson/databind/jsonschema/JsonSerializableSchema.java Outdated
* @author Ryan Heaton
* @author Tatu Saloranta
* @deprecated Since 2.2, we recommend use of external
Copy link
Member

Choose a reason for hiding this comment

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

Not true; was not deprecated in 2.2; this will be against 2.15

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this javadoc from com.fasterxml.jackson.databind.jsonschema.JsonSchema. I would have thought that JsonSerializableSchema and SchemaAware were deprecated since 2.2 along with JsonSchema but just forgot to mark with deprecated. So It should be against to 2.15?
https://github.com/FasterXML/jackson-databind/blob/2.15/src/main/java/com/fasterxml/jackson/databind/jsonschema/JsonSchema.java#L19

Copy link
Member

Choose a reason for hiding this comment

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

you can't just pretend this was deprecated back in v2.2 - you have to say 2.15

Copy link
Member

Choose a reason for hiding this comment

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

Right, @luozhenyu it was not declared as deprecated and we cannot "backdate" decision -- even if that might have been intent. I suspect it was not deprecated just to reduce noise but either way 2.15 is needed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I have changed it to 2.15

cowtowncoder
cowtowncoder reviewed Jan 20, 2023
View reviewed changes
src/main/java/com/fasterxml/jackson/databind/jsonschema/JsonSerializableSchema.java Outdated
public @interface JsonSerializableSchema
{
/**
* Marker value used to indicate that property has "no value";
* needed because annotations cannot have null as default
* value.
*/
public final static String NO_VALUE = "##irrelevant";
Copy link
Member

Choose a reason for hiding this comment

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

Please do not make unrelated changes like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got it

cowtowncoder
cowtowncoder reviewed Jan 20, 2023
View reviewed changes
src/main/java/com/fasterxml/jackson/databind/jsonschema/JsonSerializableSchema.java Outdated

/**
* Property that can be used to indicate id of the type when
* generating JSON Schema; empty String indicates that no id
* is defined.
*/
public String id() default "";

String id() default "";
Copy link
Member

Choose a reason for hiding this comment

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

Please do not make such mechanical changes; I know it is redundant but these changes obscure actual diff.

cowtowncoder
cowtowncoder reviewed Jan 20, 2023
View reviewed changes
src/main/java/com/fasterxml/jackson/databind/ser/BeanPropertyWriter.java Outdated
if (ser instanceof SchemaAware) {
schemaNode = ((SchemaAware) ser).getSchema(provider, hint,
isOptional);
if (ser instanceof com.fasterxml.jackson.databind.jsonschema.SchemaAware) {
Copy link
Member

Choose a reason for hiding this comment

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

No, please leave as-is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the import com.fasterxml.jackson.databind.jsonschema.SchemaAware; statement will generate a compiler warning of deprecated usage. So I replace it with a fully qualified class name.

Copy link
Member

Choose a reason for hiding this comment

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

just revert this change

Copy link
Member

Choose a reason for hiding this comment

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

@luozhenyu I appreciate your avoiding warnings, but is this the only way? (I forget if there was another mechanism).

@cowtowncoder
Copy link
Member

Ok; let's please start with a smaller schema just deprecating getSchema() method -- easier to review.

@luozhenyu luozhenyu force-pushed the deprecate_jsonschema branch 2 times, most recently from f90d016 to 21a0117 Compare January 20, 2023 18:07
@cowtowncoder cowtowncoder added the cla-needed PR looks good (although may also require code review), but CLA needed from submitter label Jan 28, 2023
@cowtowncoder
Copy link
Member

Ok @luozhenyu this looks good: thank you for contributing the PR!

The last before I merge this, unless I have asked and received it (apologies if so, just let me know), is Contributor License Agreement (CLA) from https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf .
It is usually easiest to print, fill and sign, scan/photo, email to info at fasterxml dot com.
Once I have it I can proceed with merging the PR. CLA only needs to be sent once and is good for all future PRs.

Looking forward to merging this!

@luozhenyu
Copy link
Contributor Author

luozhenyu commented Jan 28, 2023 •
edited

Ok @luozhenyu this looks good: thank you for contributing the PR!

The last before I merge this, unless I have asked and received it (apologies if so, just let me know), is Contributor License Agreement (CLA) from https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf . It is usually easiest to print, fill and sign, scan/photo, email to info at fasterxml dot com. Once I have it I can proceed with merging the PR. CLA only needs to be sent once and is good for all future PRs.

Looking forward to merging this!

@cowtowncoder I have signed and sent by email~

@cowtowncoder cowtowncoder added 2.15 Issues planned for 2.15 and removed cla-needed PR looks good (although may also require code review), but CLA needed from submitter labels Jan 29, 2023
@cowtowncoder cowtowncoder merged commit c55d157 into FasterXML:2.15 Jan 29, 2023
@cowtowncoder cowtowncoder changed the title Deprecate classes in package jsonschema Deprecate classes in package com.fasterxml.jackson.databind.jsonschema Jan 29, 2023
@cowtowncoder cowtowncoder added this to the 2.15.0 milestone Jan 29, 2023
cowtowncoder added a commit that referenced this pull request Jan 29, 2023
@cowtowncoder
Update release notes wrt #3745
f2cb5d5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pjfanning pjfanning pjfanning left review comments

@cowtowncoder cowtowncoder cowtowncoder approved these changes

Assignees
No one assigned
Labels
2.15 Issues planned for 2.15
Projects
None yet
Milestone
2.15.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@luozhenyu @cowtowncoder @pjfanning