feat: Add noniljson Linter to Enforce omitempty for Nullable Struct Fields

[rezkam]

Add noniljson Linter to Enforce omitempty for Nullable Struct Fields

I propose the addition of the noniljson linter to the golangci-lint project. The noniljson linter is a static analysis tool designed for Go, focusing on improving JSON serialization practices. It identifies nullable struct fields that, when marshaled into JSON, do not include the omitempty option, thus preventing these fields from being represented as null in the output.

Rationale

The inclusion of null values in JSON can introduce several challenges:

• Unexpected null Values: May lead to inconsistencies and errors in JSON consumers not designed to handle null.
• Increased Payload Size: Optional fields represented as null increase the JSON payload size, affecting transmission efficiency.
• Specification Non-compliance: Contravenes API specifications that dictate optional fields should be omitted if not present.

Solution

noniljson mitigates these issues by enforcing the addition of the omitempty tag in struct field definitions, ensuring a clean, efficient, and compliant JSON output. This linter thus serves as an invaluable tool for:

• API Development: Guarantees clean JSON output for API endpoints.
• Microservices Architecture: Optimizes inter-service communication by reducing payload size.
• Data Storage and Processing: Ensures structs are correctly prepared for JSON-based storage or processing, omitting unnecessary null values.

Impact

Integrating noniljson into golangci-lint provides developers with an automated means to enforce best serialization practices, enhancing the quality, reliability, and interoperability of Go applications with external systems and services.

Example

A common scenario identified by noniljson:

type ExampleStruct struct {
    Name    *string `json:"name"` // Issue: nullable field 'Name' should include 'omitempty' to avoid marshaling as null
    Address string  `json:"address,omitempty"`
}

[boring-cyborg]

Hey, thank you for opening your first Pull Request !

[CLAassistant]

All committers have signed the CLA.

[ldez]

In order for a pull request adding a linter to be reviewed, the linter and the PR must follow some requirements.

• The CLA must be signed

Pull Request Description

• It must have a link to the linter repository.
• It must provide a short description of the linter.

Linter

• It must not be a duplicate of another linter or a rule of a linter. (the team will help to verify that)
• It must have a valid license (AGPL is not allowed) and the file must contain the required information by the license, ex: author, year, etc.
• The linter repository must have a CI and tests.
• It must use go/analysis.
• It must have a valid tag, ex: v1.0.0, v0.1.0.
• It must not contain init().
• It must not contain panic().
• It must not contain log.fatal(), os.exit(), or similar.
• It must not modify the AST.
• It must not have false positives/negatives. (the team will help to verify that)
• It must have tests inside golangci-lint.

The Linter Tests Inside Golangci-lint

• They must have at least one std lib import.
• They must work with T=<name of the linter test file>.go make test_linters. (the team will help to verify that)

.golangci.reference.yml

• The linter must be added to the list of available linters (alphabetical case-insensitive order).
  • enable and disable options
• If the linter has a configuration, the exhaustive configuration of the linter must be added (alphabetical case-insensitive order)
  • The values must be different from the default ones.
  • The default values must be defined in a comment.
  • The option must have a short description.

Others Requirements

• The files (tests and linter) inside golangci-lint must have the same name as the linter.
• The .golangci.yml of golangci-lint itself must not be edited and the linter must not be added to this file.
• The linters must be sorted in the alphabetical order (case-insensitive) in the Manager.GetAllSupportedLinterConfigs(...) and .golangci.reference.yml.
• The load mode (WithLoadMode(...)):
  • if the linter doesn't use types: goanalysis.LoadModeSyntax
  • goanalysis.LoadModeTypesInfo required WithLoadForGoAnalysis() in the Manager.GetAllSupportedLinterConfigs(...)
• The version in WithSince(...) must be the next minor version (v1.X.0) of golangci-lint.
• WithURL() must contain the URL of the repository.

Recommendations

• The linter should not use SSA. (SSA can be a problem with generics)
• The linter repository should have a readme and linting.
• The linter should be published as a binary. (useful to diagnose bug origins)

---

The golangci-lint team will edit this comment to check the boxes before and during the review.

The code review will start after the completion of those checkboxes (except for the specific items that the team will help to verify).

If the author of the PR is a member of the golangci-lint team, he should not edit this message.

This checklist does not imply that we will accept the linter.

[ldez]

Hello,

IMHO this will lead to many false positives.
omitempty is useful only when a structure is used to marshall data.
But if a structure is only used to unmarshal data it's useless to add omitempty.
When you consume an API, you need both types of structure: request (marshall) and response (unmarshal).

Also, omitempty can be used only for optional fields.

Another problem is related to slices:

• some APIs require [] instead of null or no field for empty slices.
• some APIs require explicit null instead of no field.
  https://go.dev/play/p/6Pv8oqIuLZ1

omitempty cannot be simply put on every nilable value.

I tried the linter on a project, and it produced hundreds of false positives.

I would recommend that you create a plugin.

[rezkam]

Thank you so much for the excellent review. I appreciate the time and effort you put into it. Your analysis has made it clear that although the noniljson linter is useful in our projects, it may not be universally beneficial due to the complex configurations required to handle the diverse cases you outlined.

[ldez]

Based on my previous comment, I'm not comfortable accepting this linter.
Even if I can understand the use case, I think it will lead either to putting omitempty on "everything" or breaking API clients, for me, the first one is not a good practice, and the second one is a major problem.

I can see 3 possibilities:

• be integrated into an existing linter as an option (not by default), for example, I think to musttag because this linter detects the Marshal usage and with the support of several syntaxes (JSON, YAML, TOML, XML, ...)
• improve the linter to be smarter, by default, by doing the same thing as musttag by reporting only structures used by Marshal.
• create a plugin

[rezkam]

Thank you for your feedback! I have considered incorporating this into musttag, but my goal here is to do more than check for the presence of tags, especially with how I am handling nil values. I am not only checking whether tags are present or not(which is what I thought musttag is doing). If we proceed with this, I will thoroughly check for nullable fields in the structs intended for marshaling.
Do you have any suggestions for other areas that need to be adjusted or examined to avoid any potential issues?

[ldez]

but my goal here is to do more than check for the presence of tags, especially with how I am handling nil values.

I am not only checking whether tags are present or not

checking presence implies reading tag content, so you do the same thing but you will also check the type of the field.

I recommend opening an issue on the musttag repo and then talking with the author. Maybe he will have a good idea or maybe not, but I think it's a good first step.

Based on my previous comments, I will refuse this linter.
When your linter has a better default behavior or is integrated into another linter, we will discuss it again.