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

(c *Ctx) JSON(data interface{}) error does not respect standard json annotations ( omitempty, inline) in nested structs #1549

Closed
ghost opened this issue Sep 29, 2021 · 12 comments · Fixed by #1563

Comments

@ghost
Copy link

ghost commented Sep 29, 2021

Fiber version
v2.19.0
Issue description
context Json method ignores annotation hints like json:"token,omitempty", json:",inline" in nested structs
Code snippet

package main

import (
	"github.com/gofiber/fiber/v2"
	"log"
)

type AuthResult struct {
	Uid             string  `json:"uid"`
	NilFieldEmbed   *string `json:"nilFieldEmbed,omitempty"`
	EmptyFieldEmbed string  `json:"emptyFieldEmbed,omitempty"`
}
type SomeResponse struct {
	NormalField string  `json:"normalField,omitempty"`
	NilField    *string `json:"nilField,omitempty"`
	EmptyField  string  `json:"emptyField,omitempty"`
	*AuthResult
}

func main() {
	app := fiber.New()

	app.Get("/json", func(c *fiber.Ctx) error {
		// Create data struct:
		data := &SomeResponse{
			NormalField: "NormalField",
			AuthResult: &AuthResult{
				Uid: "UID",
			},
		}
		return c.JSON(data)
	})

	log.Fatal(app.Listen(":3000"))
}

Bug
code snipped returns json:

{
    "normalField": "NormalField",
    "uid": "UID",
    "nilFieldEmbed": null,
    "emptyFieldEmbed": ""
}

Expected:

{
  "normalField": "NormalField",
  "uid": "UID",
}
@IGLOU-EU
Copy link

Hi,

I think omitempty is for unmarshal only.

Because, myvar int == 0, but can you trust this affirmation 0 == empty ? No you can't
Or we need to replace omitemptyby a notomitempty flag, which I would find weird.

I'm wrong ? 🤔

@ghost
Copy link
Author

ghost commented Sep 29, 2021

Hi,

I think omitempty is for unmarshal only.

Because, myvar int == 0, but can you trust this affirmation 0 == empty ? No you can't Or we need to replace omitemptyby a notomitempty flag, which I would find weird.

I'm wrong ? thinking

Hi, It is a part of the spec https://pkg.go.dev/encoding/json#Marshal

@aliereno
Copy link
Contributor

Hi, I just tried your code and it gives your expected response. Am I missing something?

image

% cat go.mod 
module main

go 1.17

require github.com/gofiber/fiber/v2 v2.19.0

require (
	github.com/andybalholm/brotli v1.0.2 // indirect
	github.com/klauspost/compress v1.13.4 // indirect
	github.com/valyala/bytebufferpool v1.0.0 // indirect
	github.com/valyala/fasthttp v1.29.0 // indirect
	github.com/valyala/tcplisten v1.0.0 // indirect
	golang.org/x/sys v0.0.0-20210514084401-e8d321eab015 // indirect
)

@ghost ghost changed the title (c *Ctx) JSON(data interface{}) error does not respect standard json annotations ( omitempty, inline) (c *Ctx) JSON(data interface{}) error does not respect standard json annotations ( omitempty, inline) in nested structs Sep 29, 2021
@ghost
Copy link
Author

ghost commented Sep 29, 2021

Hi, I just tried your code and it gives your expected response. Am I missing something?

image

% cat go.mod 
module main

go 1.17

require github.com/gofiber/fiber/v2 v2.19.0

require (
	github.com/andybalholm/brotli v1.0.2 // indirect
	github.com/klauspost/compress v1.13.4 // indirect
	github.com/valyala/bytebufferpool v1.0.0 // indirect
	github.com/valyala/fasthttp v1.29.0 // indirect
	github.com/valyala/tcplisten v1.0.0 // indirect
	golang.org/x/sys v0.0.0-20210514084401-e8d321eab015 // indirect
)

Thank you you are right! Bug reproduced in nested structs only.
Updated description and title

@liaohongxing
Copy link
Contributor

internal encoding/json needs to be updated

@kallydev
Copy link
Contributor

kallydev commented Oct 1, 2021

Hello @ReneWerner87, I've done some troubleshooting on this issue.

When using nested struct with pointer type, the pointer is a wild pointer due to some incorrect code, so f.empty(v) will return true or false at random.

if f.omitempty && f.empty(v) {
continue
}

I noticed that upstream https://github.com/segmentio/encoding also has this problem, can I migrate it to https://github.com/json-iterator/go via PR?

@efectn
Copy link
Member

efectn commented Oct 1, 2021

Hello @ReneWerner87, I've done some troubleshooting on this issue.

When using nested struct with pointer type, the pointer is a wild pointer due to some incorrect code, so f.empty(v) will return true or false at random.

if f.omitempty && f.empty(v) {
continue
}

I noticed that upstream https://github.com/segmentio/encoding also has this problem, can I migrate it to https://github.com/json-iterator/go via PR?

I think using https://github.com/goccy/go-json or https://github.com/json-iterator/go good idea. We can do benchmark to compare performance.

@hi019
Copy link
Contributor

hi019 commented Oct 2, 2021

Looks like it's related to this

segmentio/encoding#63

@ReneWerner87
Copy link
Member

you are welcome to test other json libraries, if the benchmarks look good, we can also switch to them

after the benchmarks, of course, we need to shed some light on how trustworthy the source is

@efectn
Copy link
Member

efectn commented Oct 4, 2021

Benchmark results: https://termbin.com/j4j19

Also, go-json seems has good compatibility with encoding/json stds.

@ReneWerner87
Copy link
Member

ok thx,
we have taken a look at it and also at the github pages and internally allow the switch to go-json as there is more activity here as well
https://github.com/goccy/go-json

would be cool if someone can provide a pull request for it, please in the way the other library was already added, so a copy with comments and without the unittests of the library itself, as these were already checked in main repository

maybe you have to search the readme's if there is something to change here
and remove the old one

@efectn
Copy link
Member

efectn commented Oct 5, 2021

ok thx, we have taken a look at it and also at the github pages and internally allow the switch to go-json as there is more activity here as well https://github.com/goccy/go-json

would be cool if someone can provide a pull request for it, please in the way the other library was already added, so a copy with comments and without the unittests of the library itself, as these were already checked in main repository

maybe you have to search the readme's if there is something to change here and remove the old one

What should do we iso8601 and ascii dirs? Should we change json dir only?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants