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

Allow EOF IO passed to JSON::PullParser.new #10864

Merged
merged 3 commits into from
Jul 26, 2021

Conversation

Blacksmoke16
Copy link
Member

@Blacksmoke16 Blacksmoke16 commented Jul 1, 2021

EOF is a valid pull parser token type. Because of this, it makes sense to set the initial kind to EOF if that's the initial state of the input versus raising an exception.

Discovered via Blacksmoke16/oq#82

@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:serialization labels Jul 1, 2021
src/json/pull_parser.cr Outdated Show resolved Hide resolved
@Blacksmoke16
Copy link
Member Author

This is good for another review @asterite.

Copy link
Member

@asterite asterite 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. On second thought, what's the use case for this? An empty json is invalid. I think json pull parser only produces valid json expressions, and eof is not one of them.

@Blacksmoke16
Copy link
Member Author

@asterite The use case is for oq in the case where the jq filter doesn't output anything. E.g. jq '.[] | select(.name != "foo")' <<<$'[{"name":"foo"}]'. When using a filter like this with oq, and with the output format specified to something other than JSON, the pull parser is failing because the IO is already essentially empty, which is this line: https://github.com/Blacksmoke16/oq/blob/master/src/converters/yaml.cr#L11

oq -i yaml -o yaml '.[] | select(.name != "foo")' <<<$'- name: foo'
oq error: Unexpected token: <EOF> at line 1, column 1

The solution in oq land is of course to just return early if there is no data to process. However I don't think there's a way to know that the related IO has no data, so this seemed like the most logical solution. I suppose I could just rescue this exception but 🤷.

@straight-shoota
Copy link
Member

@asterite This change does not parse an empty document as valid. It just makes sure to properly forward the information that it has reached EOF on the input stream.

@asterite
Copy link
Member

asterite commented Jul 5, 2021

Thanks! Feel free to merge.

@straight-shoota
Copy link
Member

straight-shoota commented Jul 5, 2021

I'm actually unsure if this qualifies as a bug-fix or should wait for the end of the feature freeze.
(Thumbs up for bug fix, thumbs down for wait)

@beta-ziliani
Copy link
Member

I need an emoji for a horizontal thumb :-D really, both interpretations are valid here. I vote for merging it. BTW @Blacksmoke16 thanks for refactoring the case, that's also a nice improvement.

@straight-shoota straight-shoota added this to the 1.1.0 milestone Jul 9, 2021
@straight-shoota
Copy link
Member

straight-shoota commented Jul 12, 2021

I think I'd rather postpone this to 1.2. We're about to release 1.1 and there's an off chance that this might have any unforeseeable effect. It's not an urgent bug fix and easy to work around.

@straight-shoota straight-shoota modified the milestones: 1.1.0, 1.2.0 Jul 12, 2021
@straight-shoota straight-shoota merged commit 87b4e9e into crystal-lang:master Jul 26, 2021
@Blacksmoke16 Blacksmoke16 deleted the parse-empty-io branch July 26, 2021 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:serialization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants