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

httpjson: Add toJSON helper function #32472

Merged
merged 4 commits into from
Aug 21, 2022

Conversation

andrewkroh
Copy link
Member

@andrewkroh andrewkroh commented Jul 22, 2022

What does this PR do?

toJSON converts the given structure into a JSON string.

Example usage:

  response.pagination:
    - set:
        target: body.pagingIdentifiers
        value: '[[ toJSON .last_response.body.pagingIdentifiers ]]'
        value_type: json
        fail_on_template_error: true

Why is it important?

I didn't find any way to use the set transform to add an existing object to the output. If you omit the toJSON and reference an object in a template then the output is a stringified version of the map like map[hashes:map[13b97806f9067edfc11359732d8c7d63:0] which is not what you want when constructing a JSON request body.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Jul 22, 2022
@andrewkroh andrewkroh force-pushed the filebeat/feature/httpjson-tojson branch from 3cb9c70 to d0852d3 Compare July 22, 2022 18:33
@andrewkroh andrewkroh marked this pull request as ready for review July 22, 2022 18:33
@andrewkroh andrewkroh requested a review from a team as a code owner July 22, 2022 18:33
@elasticmachine
Copy link
Collaborator

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

toJSON converts the given structure into a JSON string.

Example usage:

  response.pagination:
    - set:
        target: body.pagingIdentifiers
        value: '[[ toJSON .last_response.body.pagingIdentifiers ]]'
        value_type: json
        fail_on_template_error: true
@andrewkroh andrewkroh force-pushed the filebeat/feature/httpjson-tojson branch from d0852d3 to f5a431c Compare July 22, 2022 18:38
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jul 22, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-08-19T15:05:39.638+0000

  • Duration: 81 min 45 sec

Test stats 🧪

Test Results
Failed 0
Passed 2176
Skipped 166
Total 2342

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

Copy link
Contributor

@efd6 efd6 left a comment

Choose a reason for hiding this comment

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

It seems to me that this should be made to work without needing this. In the case that top level maps are mapstr.M then this is true, so it would be worth finding out why the map that prompted the fix did not end up being that type.

I'm struggling to find where that could be happening or to make the test fail, e.g. this passes

		{
			name:  "func toJSON",
			value: "[[ .last_response.body.paginationParams.m.m ]]",
			paramCtx: &transformContext{
				firstEvent:   &mapstr.M{},
				lastEvent:    &mapstr.M{},
				lastResponse: newTestResponse(map[string]interface{}{"paginationParams": map[string]interface{}{"m": map[string]interface{}{"m": map[string]interface{}{"id": 1234}}}}, nil, ""),
			},
			paramTr:     transformable{},
			expectedVal: `{"id":1234}`,
		},

@@ -574,6 +574,17 @@ func TestValueTpl(t *testing.T) {
paramTr: transformable{},
expectedVal: "my value",
},
{
name: "func toJSON",
value: "[[ toJSON .last_response.body.paginationParams ]]",
Copy link
Contributor

Choose a reason for hiding this comment

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

This test passes without the toJSON call since the parameter is a mapstr.M whose String method renders to JSON.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I changed the test such that it will fail without toJSON. The code uses mapstr.M.Clone to convert inner objects to mapstr.M, but it does not look inside of []interface{} for maps. So without toJSON you get

        	Error Trace:	value_tpl_test.go:609
        	Error:      	Not equal: 
        	            	expected: "[{\"id\":1234}]"
        	            	actual  : "[map[id:1234]]"

@andrewkroh
Copy link
Member Author

andrewkroh commented Jul 24, 2022

Interesting, one clue I have is that this was only needed when processing a response body that is an array of objects. If the response body is an object then it was working fine without running the toJSON on the value.

In the case that top level maps are mapstr.M then this is true

So perhaps when the response is an array then the inner objects are left as map[string]interface{} instead of being converted to mapstr.M that has a String() method that returns JSON?

@efd6
Copy link
Contributor

efd6 commented Jul 24, 2022

Yes, that explains it. This would be fixed if mapstr.M did work to convert map[string]interface{} elements in a []map[string]interface{}. This can be done unsafely in mapstr.mapFind with zero cost, but ... unsafe. The alternative to fix it would be to fully implement json.Unmarshaler for mapstr.M so that the abstraction is complete.

Alternatively this, but with a failable test.

@mergify
Copy link
Contributor

mergify bot commented Aug 16, 2022

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @andrewkroh? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)
    To fixup this pull request, you need to add the backport labels for the needed
    branches, such as:
  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

Instead of calling toJSON on a mapstr.M, which would already return JSON because
mapstr.M implements fmt.Stringer to output JSON, call it on a map[string]interface{}.
@andrewkroh
Copy link
Member Author

andrewkroh commented Aug 19, 2022

In decodeAsJSON, if we apply a conversion to the []interface{} that translates the map[string]interface{} values to mapstr.M then things start working. But this has a cascading effect since other parts of the code are not expecting to find mapstr.M.

If I knew all the places where data originates then I would make sure they it were all updated to only produce mapstr.M. I raised and issue to consider making some changes to elastic/elastic-agent-libs#75. But I think I would like to proceed with toJSON since it's simple and does not preclude other solutions.

Conversion hack: httpjson-deep-clone-mapstr.patch.txt

@andrewkroh andrewkroh requested a review from efd6 August 19, 2022 17:18
@andrewkroh andrewkroh merged commit 396af77 into elastic:main Aug 21, 2022
@andrewkroh andrewkroh added the backport-v8.4.0 Automated backport with mergify label Aug 21, 2022
mergify bot pushed a commit that referenced this pull request Aug 21, 2022
toJSON converts the given structure into a JSON string.

Example usage:

  response.pagination:
    - set:
        target: body.pagingIdentifiers
        value: '[[ toJSON .last_response.body.pagingIdentifiers ]]'
        value_type: json
        fail_on_template_error: true

(cherry picked from commit 396af77)
andrewkroh added a commit that referenced this pull request Aug 21, 2022
toJSON converts the given structure into a JSON string.

Example usage:

  response.pagination:
    - set:
        target: body.pagingIdentifiers
        value: '[[ toJSON .last_response.body.pagingIdentifiers ]]'
        value_type: json
        fail_on_template_error: true

(cherry picked from commit 396af77)

Co-authored-by: Andrew Kroh <[email protected]>
chrisberkhout pushed a commit that referenced this pull request Jun 1, 2023
toJSON converts the given structure into a JSON string.

Example usage:

  response.pagination:
    - set:
        target: body.pagingIdentifiers
        value: '[[ toJSON .last_response.body.pagingIdentifiers ]]'
        value_type: json
        fail_on_template_error: true
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v8.4.0 Automated backport with mergify enhancement Filebeat Filebeat
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants