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

arichapn · 2023-12-07T13:54:29Z

Added are:

JsonCreator.Mode
JsonFormat.Shape
JsonInclude.Include
JsonProperty.Access

github-actions · 2023-12-07T13:56:03Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

EnricSala · 2023-12-07T13:59:09Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImport.java

@@ -57,6 +57,7 @@ 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",


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.

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.

I agree with your proposal 👍

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:

JsonFormat.Shape

JsonInclude.Include

JsonProperty.Access

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?

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).

Just saw Enric's emoji response. 😄

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

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.

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. 🙂

Added, let me know what you think. 👍

github-actions · 2023-12-07T14:19:48Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-12-08T09:32:35Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

Thanks for filing a PR @arichapn 🚀!

rickie · 2023-12-11T20:46:33Z

Suggested commit message:

Require static import of various `com.fasterxml.jackson.annotation` members (#910)

github-actions · 2023-12-11T20:48:11Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202

Rebased and added some tweaks.

Suggested commit message:

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

Types to be imported statically:
- `com.fasterxml.jackson.annotation.JsonCreator.Mode`
- `com.fasterxml.jackson.annotation.JsonFormat.Shape`
- `com.fasterxml.jackson.annotation.JsonInclude.Include`
- `com.fasterxml.jackson.annotation.JsonProperty.Access`

Stephan202 · 2023-12-16T14:52:38Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportTest.java

@@ -109,6 +110,12 @@ void identification() {
            "  @UseImportPolicy(ImportPolicy.IMPORT_TOP_LEVEL)",
            "  void refasterAfterTemplate() {}",
            "",
+            "  // BUG: Diagnostic contains:",
+            "  @JsonCreator(mode = JsonCreator.Mode.DELEGATING)",
+            "  private static A create(int a) {",


For consistency with the method above:

Suggested change

" private static A create(int a) {",

" private static A jsonCreator(int a) {",

(To better match the declaration order in StaticImport I'll move this up a bit.)

github-actions · 2023-12-16T15:03:45Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

…JsonInclude.Include,JsonProperty.Access}`

github-actions · 2023-12-16T16:47:51Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

sonarcloud · 2023-12-16T16:47:53Z

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

arichapn requested a review from rickie December 7, 2023 13:55

EnricSala reviewed Dec 7, 2023

View reviewed changes

rickie added the improvement label Dec 8, 2023

rickie added this to the 0.15.0 milestone Dec 8, 2023

arichapn changed the title ~~StaticImport: add com.fasterxml.jackson.annotation.JsonCreator.Mode~~ StaticImport: add various com.fasterxml.jackson.annotation values Dec 8, 2023

rickie approved these changes Dec 11, 2023

View reviewed changes

rickie force-pushed the arichapn/json-creator-mode-static-imports branch from 320f9b6 to 8a913b7 Compare December 11, 2023 20:46

rickie requested a review from Stephan202 December 11, 2023 20:46

Stephan202 force-pushed the arichapn/json-creator-mode-static-imports branch from 8a913b7 to f8cad7c Compare December 16, 2023 15:00

Stephan202 approved these changes Dec 16, 2023

View reviewed changes

arichapn and others added 4 commits December 16, 2023 17:46

StaticImport: add com.fasterxml.jackson.annotation.JsonCreator.Mode

00b3d79

Update test code to reflect Google code style

37588e5

StaticImport add `com.fasterxml.jackson.annotation.{JsonFormat.Shape,…

a762ac1

…JsonInclude.Include,JsonProperty.Access}`

Suggestions

03a43a3

rickie force-pushed the arichapn/json-creator-mode-static-imports branch from f8cad7c to 03a43a3 Compare December 16, 2023 16:46

rickie merged commit 7c2078b into master Dec 18, 2023
16 checks passed

rickie deleted the arichapn/json-creator-mode-static-imports branch December 18, 2023 06:17

Stephan202 changed the title ~~StaticImport: add various com.fasterxml.jackson.annotation values~~ Require static importing of some com.fasterxml.jackson.annotation enums Feb 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

arichapn commented Dec 7, 2023 •

edited

github-actions bot commented Dec 7, 2023

EnricSala Dec 7, 2023

rickie Dec 7, 2023

EnricSala Dec 7, 2023

arichapn Dec 7, 2023

arichapn Dec 7, 2023

arichapn Dec 8, 2023

rickie Dec 8, 2023

arichapn Dec 8, 2023

arichapn Dec 8, 2023

github-actions bot commented Dec 7, 2023

github-actions bot commented Dec 8, 2023

rickie left a comment

rickie commented Dec 11, 2023

github-actions bot commented Dec 11, 2023

Stephan202 left a comment

Stephan202 Dec 16, 2023

Stephan202 Dec 16, 2023

github-actions bot commented Dec 16, 2023

github-actions bot commented Dec 16, 2023

sonarcloud bot commented Dec 16, 2023

	" private static A create(int a) {",
	" private static A jsonCreator(int a) {",

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

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

Conversation

arichapn commented Dec 7, 2023 • edited

github-actions bot commented Dec 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 7, 2023

github-actions bot commented Dec 8, 2023

rickie left a comment

Choose a reason for hiding this comment

rickie commented Dec 11, 2023

github-actions bot commented Dec 11, 2023

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 16, 2023

github-actions bot commented Dec 16, 2023

sonarcloud bot commented Dec 16, 2023

Quality Gate passed

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

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

arichapn commented Dec 7, 2023 •

edited