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

Require static importing of some com.fasterxml.jackson.annotation enums #910

Merged
merged 4 commits into from
Dec 18, 2023
Merged

Require static importing of some com.fasterxml.jackson.annotation enums #910

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
00b3d79
StaticImport: add `com.fasterxml.jackson.annotation.JsonCreator.Mode`
arichapn Dec 7, 2023
37588e5
Update test code to reflect Google code style
arichapn Dec 7, 2023
a762ac1
StaticImport add `com.fasterxml.jackson.annotation.{JsonFormat.Shape,…
arichapn Dec 8, 2023
03a43a3
Suggestions
Stephan202 Dec 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ public final class StaticImport extends BugChecker implements MemberSelectTreeMa
@VisibleForTesting
static final ImmutableSet<String> STATIC_IMPORT_CANDIDATE_TYPES =
ImmutableSet.of(
"com.fasterxml.jackson.annotation.JsonCreator.Mode",
Copy link

Choose a reason for hiding this comment

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

There's more cases like:

  • @JsonFormat.Shape.
  • @JsonTypeInfo.As.
  • @JsonTypeInfo.Id.
  • @JsonSubTypes.Type.
  • (probably more)

Do you think we could find a way to handle these in bulk @rickie? :)

Or would you recommend listing (and testing) each one individually.

Copy link
Member

Choose a reason for hiding this comment

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

No we do not need to test all of them. The check is already sufficiently tested.

We can simply add the members to the list. I'm not sure if we should just add all of them. We only add the ones where statically importing doesn't make the code "less readable/understandable", as in, we do not lose any context on what is happening.

  • The JsonFormat.Shape seems like a no-brainer that we should add.
  • For .Id I"m not sure: @JsonTypeInfo(use = JsonTypeInfo.Id.CLASS, I'd say, JsonTypeInfo can be omitted but the Id part is maybe still "fine" to have? Same holds for:
@JsonTypeInfo(
    use = NAME,
    include = JsonTypeInfo.As.EXISTING_PROPERTY,

We can drop JsonTypeInfo but having .As is maybe better? This is a bit up for discussion I feel. WDYT?

  • I'd argue that @Type is not that clear where it comes from. On the other hand, all usages are only nested in the @JsonSubTypes annotation (I think) and in that case it would make sense.

Copy link

Choose a reason for hiding this comment

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

I agree with your proposal 👍

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 took the annotations from the docs, filtered down to the inner classes, and then did quick scans of some uses in public code on GitHub.

If we ignore JsonCreator.Mode as it's already addressed, and filter down to only the cases where I primarily saw uses where the qualification provides superfluous context, these are what we're left with:

I'd vote for including those in this PR with the same approach as what's currently here for JsonCreator.Mode.

I also looked at the other types Enric mentioned, and agree with Rick:

  • For JsonTypeInfo.As and JsonTypeInfo.Id, I think we should keep around the As and Id bits because of code like this
  • For JsonSubTypes.Type, I think we should keep around the Type bit because of code like this

So to summarise, my vote is to add the following changes here:

  • JsonFormat.Shape.X --> X
  • JsonInclude.Include.X --> X
  • JsonProperty.Access.X --> X
  • JsonTypeInfo.As.X --> As.X
  • JsonTypeInfo.Id.X --> Id.X
  • JsonSubTypes.Type.X --> Type.X

What do you guys think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And as for testing, I'd say what we already have provides sufficient confidence for the cases where we're removing all qualification. But it's probably a good idea to add a test for one of the cases where we're leaving some qualification (e.g. JsonTypeInfo.As.X --> As.X).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just saw Enric's emoji response. 😄

Curious to hear your thoughts too @rickie, if this all sounds good then I'll proceed. 👍

Copy link
Member

Choose a reason for hiding this comment

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

I completely agree with the proposal of which ones to statically import. W.r.t. the ones that are still qualified but not statically imported, I'm not sure if that would fit in this check TBH. I think that should go in the NonStaticImport as there we qualify which types to qualify instead of statically import. Although I'm not sure if that also does this rewrite: JsonTypeInfo.Id.X to Id.X but it does make X -> Id.X. 😄

Nonetheless, I'd say it's worth checking out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, in that case I'll just add the static import options to this PR.

For the imports which remain (at least partially) qualified, I can keep it on my TODO list to check those out. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added, let me know what you think. 👍

"com.fasterxml.jackson.annotation.JsonFormat.Shape",
"com.fasterxml.jackson.annotation.JsonInclude.Include",
"com.fasterxml.jackson.annotation.JsonProperty.Access",
"com.google.common.base.Preconditions",
"com.google.common.base.Predicates",
"com.google.common.base.Verify",
Expand Down
20 changes: 20 additions & 0 deletions error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ void identification() {
"import static java.util.function.Predicate.not;",
"import static org.springframework.boot.test.context.SpringBootTest.WebEnvironment.RANDOM_PORT;",
"",
"import com.fasterxml.jackson.annotation.JsonCreator;",
"import com.google.common.base.Predicates;",
"import com.google.common.collect.ImmutableMap;",
"import com.google.common.collect.ImmutableMultiset;",
Expand Down Expand Up @@ -106,6 +107,12 @@ void identification() {
" }",
"",
" // BUG: Diagnostic contains:",
" @JsonCreator(mode = JsonCreator.Mode.DELEGATING)",
" private static A jsonCreator(int a) {",
" return new A();",
" }",
"",
" // BUG: Diagnostic contains:",
" @UseImportPolicy(ImportPolicy.IMPORT_TOP_LEVEL)",
" void refasterAfterTemplate() {}",
"",
Expand All @@ -121,6 +128,7 @@ void replacement() {
"A.java",
"import static java.util.function.Predicate.not;",
"",
"import com.fasterxml.jackson.annotation.JsonCreator;",
"import com.google.common.base.Predicates;",
"import com.google.common.collect.ImmutableMap;",
"import com.google.common.collect.ImmutableSet;",
Expand Down Expand Up @@ -177,6 +185,11 @@ void replacement() {
" @DateTimeFormat(iso = ISO.DATE_TIME) String dateTime,",
" @DateTimeFormat(iso = ISO.TIME) String time) {}",
"",
" @JsonCreator(mode = JsonCreator.Mode.DELEGATING)",
" private static A jsonCreator(int a) {",
" return new A();",
" }",
"",
" @BugPattern(",
" summary = \"\",",
" linkType = BugPattern.LinkType.NONE,",
Expand All @@ -189,6 +202,7 @@ void replacement() {
"}")
.addOutputLines(
"A.java",
"import static com.fasterxml.jackson.annotation.JsonCreator.Mode.DELEGATING;",
"import static com.google.common.collect.ImmutableMap.toImmutableMap;",
"import static com.google.common.collect.ImmutableSet.toImmutableSet;",
"import static com.google.errorprone.BugPattern.LinkType.NONE;",
Expand All @@ -208,6 +222,7 @@ void replacement() {
"import static org.springframework.http.MediaType.APPLICATION_XHTML_XML;",
"import static org.springframework.http.MediaType.TEXT_HTML;",
"",
"import com.fasterxml.jackson.annotation.JsonCreator;",
"import com.google.common.base.Predicates;",
"import com.google.common.collect.ImmutableMap;",
"import com.google.common.collect.ImmutableSet;",
Expand Down Expand Up @@ -261,6 +276,11 @@ void replacement() {
" @DateTimeFormat(iso = DATE_TIME) String dateTime,",
" @DateTimeFormat(iso = TIME) String time) {}",
"",
" @JsonCreator(mode = DELEGATING)",
" private static A jsonCreator(int a) {",
" return new A();",
" }",
"",
" @BugPattern(summary = \"\", linkType = NONE, severity = SUGGESTION, tags = SIMPLIFICATION)",
" static final class TestBugPattern {}",
"",
Expand Down