Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

fix(parsers.json_v2): Remove BOM before parsing #11926

Merged
merged 2 commits into from
Oct 12, 2022
Merged

fix(parsers.json_v2): Remove BOM before parsing #11926

merged 2 commits into from
Oct 12, 2022

Conversation

powersj
Copy link
Contributor

@powersj powersj commented Oct 3, 2022

The file input will remove a bom from a file before attempting to parse it. If a body has the bom it will fail to parse.

fixes: #11916

@powersj
fix(inputs.http): remove bom from response body
1f106d5 
The file input will remove a bom from a file before attmepting to parse
it. If a body has the bom it will fail to parse.

fixes: influxdata#11916
@telegraf-tiger telegraf-tiger bot added fix pr to fix corresponding bug plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Oct 3, 2022
@powersj powersj mentioned this pull request Oct 3, 2022
Closed
srebhan
srebhan approved these changes Oct 4, 2022
View reviewed changes
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for putting this up @powersj! Do we need this for all plugins, i.e. should we maybe move this to the JSON parser(s) instead?

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Oct 4, 2022
@powersj
Copy link
Contributor Author

powersj commented Oct 4, 2022

Do we need this for all plugins, i.e. should we maybe move this to the JSON parser(s) instead?

This crossed my mind since we already have it in two spots now. I was slightly worried about breaking something, but it seems data with a BOM will just not work. I will push a new commit to do this for every parse. Any concerns about doing it that way?

@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Oct 4, 2022

Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1%)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB RPM TAR GZ ZIP
amd64.deb aarch64.rpm darwin_amd64.tar.gz windows_amd64.zip
arm64.deb armel.rpm darwin_arm64.tar.gz windows_i386.zip
armel.deb armv6hl.rpm freebsd_amd64.tar.gz
armhf.deb i386.rpm freebsd_armv7.tar.gz
i386.deb ppc64le.rpm freebsd_i386.tar.gz
mips.deb riscv64.rpm linux_amd64.tar.gz
mipsel.deb s390x.rpm linux_arm64.tar.gz
ppc64el.deb x86_64.rpm linux_armel.tar.gz
riscv64.deb linux_armhf.tar.gz
s390x.deb linux_i386.tar.gz
linux_mips.tar.gz
linux_mipsel.tar.gz
linux_ppc64le.tar.gz
linux_riscv64.tar.gz
linux_s390x.tar.gz
static_linux_amd64.tar.gz

srebhan
srebhan approved these changes Oct 4, 2022
View reviewed changes
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks @powersj! I think we should centralize this function, e.g. in internal as we already have at least 4 users...

plugins/parsers/json_v2/parser.go
Comment on lines +74 to +76
reader := strings.NewReader(string(input))
body, _ := utfbom.Skip(reader)
input, err := io.ReadAll(body)
Copy link
Member

Choose a reason for hiding this comment

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

Can we please make this an internal function? We trim BOMs in inputs.file, inputs.tail, tools/custom_builder and last but not least config/config.go (trimBOM()) and I guess we should use this new function in all those places (later).

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be useful to have this in telegraf shared code (like internal) but we could merge this first and factor it out into internal in a follow-up PR.

@srebhan srebhan self-assigned this Oct 4, 2022
@srebhan srebhan changed the title fix(inputs.http): remove bom from response body fix(parsers.json): remove bom from response body Oct 4, 2022
@srebhan srebhan changed the title fix(parsers.json): remove bom from response body fix(parsers.json_v2): remove bom from response body Oct 4, 2022
@srebhan srebhan added area/json json and json_v2 parser/serialiser related plugin/parser 1. Request for new parser plugins 2. Issues/PRs that are related to parser plugins and removed plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Oct 4, 2022
@srebhan srebhan changed the title fix(parsers.json_v2): remove bom from response body fix(parsers.json_v2): Remove bom from response body Oct 4, 2022
@srebhan srebhan changed the title fix(parsers.json_v2): Remove bom from response body fix(parsers.json_v2): Remove BOM before parsing Oct 4, 2022
@powersj powersj merged commit 7c1d175 into influxdata:master Oct 12, 2022
@powersj powersj deleted the fix/11916 branch October 12, 2022 19:09
dba-leshop pushed a commit to dba-leshop/telegraf that referenced this pull request Oct 30, 2022
@powersj @dba-leshop
fix(parsers.json_v2): Remove BOM before parsing (influxdata#11926)
433556d
popey pushed a commit that referenced this pull request Nov 2, 2022
@powersj
fix(parsers.json_v2): Remove BOM before parsing (#11926)
dc70f3a 
(cherry picked from commit 7c1d175)
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@reimda reimda reimda left review comments

@srebhan srebhan srebhan approved these changes

Assignees

@srebhan srebhan

Labels
area/json json and json_v2 parser/serialiser related fix pr to fix corresponding bug plugin/parser 1. Request for new parser plugins 2. Issues/PRs that are related to parser plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

JSON_v2 parses OK from file, but not from HTTP
3 participants
@powersj @reimda @srebhan