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 encoding bug when pushing a malformed payload #52

Merged
merged 3 commits into from Mar 1, 2017

Conversation

@fern4lvarez
Copy link
Owner

fern4lvarez commented Mar 1, 2017

Before this fix, if you tried to PUSH an element using some
of the following payloads:

  • {"elent":8}
  • {"foo":8}
  • {1:8}

The server wouldn't complain when parsing them, and would store a
null on top of the Stack. Same behavior when trying to set some config
value.

With this fix, we now check that the payload contains the word element
as key. Otherwise, we return an error when decoding it, that will become
a 400 Bad Request in pilad.

fern4lvarez added 3 commits Mar 1, 2017
Before this fix, if you tried to `PUSH` an element using some
of the following payloads:

* `{"elent":8}`
* `{"foo":8}`
* `{1:8}`

The server wouldn't complain when parsing them, and would store a
`null` on top of the Stack. Same behavior when trying to set some config
value.

With this fix, we now check that the payload contains the word `element`
as key. Otherwise, we return an error when decoding it, that will become
a `400 Bad Request` in `pilad`.
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 1, 2017

Codecov Report

Merging #52 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #52   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          18     18           
  Lines         674    766   +92     
=====================================
+ Hits          674    766   +92
Impacted Files Coverage Δ
pila/stack.go 100% <100%> (ø)
pilad/conn.go 100% <100%> (ø)
pilad/router.go 100% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7443a83...9b384ce. Read the comment docs.

@fern4lvarez fern4lvarez added the bug label Mar 1, 2017
fern4lvarez added a commit to oscillatingworks/docs.piladb.org that referenced this pull request Mar 1, 2017
@fern4lvarez fern4lvarez merged commit 509f9b1 into master Mar 1, 2017
5 checks passed
5 checks passed
codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to 7443a83
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
hound No violations found. Woof!
@fern4lvarez fern4lvarez deleted the fix-invalid-element-payload branch Mar 1, 2017
@skolodyazhnyy

This comment has been minimized.

Copy link

skolodyazhnyy commented Mar 22, 2017

Would this work now?

{
    "element": "hello"
}

Its valid JSON payload with proper "element" field :)

@fern4lvarez

This comment has been minimized.

Copy link
Owner Author

fern4lvarez commented Mar 22, 2017

@skolodyazhnyy It wouldn't work, as the server expects the JSON payload to be trimmed of whitespaces and new lines. So in this case the client would need to adapt the payload, at least until we make the payload parser a bit smarter.

The thing is that with your example you would get an error with a proper message, whereas previously a null object would be pushed, which is a big bug.

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.