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

Fix #881 (#464): Added JSON encoder to disable escaping html symbols #887

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Moskovych
Copy link

@Moskovych Moskovych commented Jun 8, 2021

Fixes #881
Fixes #464

@Moskovych
Copy link
Author

@felixfontein , @autrilla , could you please review PR and approve builds?
Any additional changes are required? More tests?
Functionality is not changed - as you said, output file is semantically the same.

Copy link
Contributor

@autrilla autrilla left a comment

Choose a reason for hiding this comment

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

LGTM. Because this is technically a breaking change, I'd like @ajvb's LGTM too. Like I've mentioned before, I think this kind of breaking change is not really breaking, since we make no guarantees about the syntax of the output, and only about the semantics.

@autrilla
Copy link
Contributor

A test for this would be nice to have

@Moskovych
Copy link
Author

@autrilla A test for this would be nice to have

Ok, will work on it.

@autrilla autrilla requested a review from ajvb June 11, 2021 10:09
@autrilla
Copy link
Contributor

Hmmm, looks like the tests are broken

@Moskovych
Copy link
Author

@autrilla , and looks like it's pgp one, which I didn't changed.

@Moskovych
Copy link
Author

I can't proceed with adding new tests as existing are not working properly (see #882)

@tanandy
Copy link

tanandy commented Dec 1, 2021

hi guys we have the same issue, any updates here ?

@Moskovych
Copy link
Author

@tanandy , sorry for delaying, but this PR still require unit test to be covered. I'll try to add them asap.

@ajvb ajvb added this to the v3.8.0 milestone Mar 3, 2022
@akshaypatidar1999
Copy link

@ajvb @autrilla Can you please release this? We are also facing similar issue

@Moskovych
Copy link
Author

Still blocked by #977.

@hiddeco
Copy link
Member

hiddeco commented Jul 28, 2023

Have you tried one of the keys from this history tree? https://github.com/getsops/sops/commits/main/pgp/sops_functional_tests_key.asc

@Moskovych
Copy link
Author

@hiddeco , thanks for pointing me out, but correct me if I'm wrong:
to fix the tests: stores/json/store_test.go
I need to decrypt the file: stores/json/test_resources/example.json
where it contains aws kms key (for which I don't have access) or gpg E5297818703249D0C60E19E6824612478D1A4CCD

From what I see, there are no such key (the last one, gpg), even in history of repository.

Did I missed something?

@felixfontein
Copy link
Contributor

stores/json/test_resources/example.json isn't used in the tests.

@Moskovych
Copy link
Author

@felixfontein , github workflows are not running without any approvals. Can you help with it?

buffer := &bytes.Buffer{}
encoder := json.NewEncoder(buffer)
encoder.SetEscapeHTML(false)
err := encoder.Encode(v)
Copy link
Contributor

Choose a reason for hiding this comment

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

Encode appends a newline (https://pkg.go.dev/encoding/json#Encoder.Encode), which json.Marshal apparently does not do. (This makes the unit tests fail.) This is apparently because Encoder is for JSON streams, while Marshal produces a single JSON file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Simply adding code which removes a trailing newline (if exists) probably suffices to fix this. I don't see another way to do this with Go's API.

Copy link
Author

Choose a reason for hiding this comment

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

Shouldn't we stick with newline in new files instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

This function is called from encodeValue and used during serialization of any JSON value, so if you use this to create JSON of a map, you get someting like

{
  "foo"
: "bar"
,
  "baz"
: 42
,
  "bam"
: null
}

@felixfontein felixfontein modified the milestones: v3.9.0, 3.10.0 Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants