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

Catch invalid query params #267

Merged
merged 4 commits into from Apr 22, 2018

Conversation

Projects
None yet
3 participants
@Lvl4Sword
Contributor

Lvl4Sword commented Apr 20, 2018

This way there isn't a server error when trying negative counts ( ?count=-1 for example )

Catch negative count
This way there isn't a server error when trying negative counts ( ?count=-1 for example )
@dermoth

This comment has been minimized.

Member

dermoth commented Apr 22, 2018

Now you've written it I guess I can merge it... what happens anyway when count < 0? shows nothing? Exception?

If count is <= 0 or >= 2017, count = 20. If 'hi'..
is a string or float, set to 19 ( which would show 0 - 19, 20 total )
@Lvl4Sword

This comment has been minimized.

Contributor

Lvl4Sword commented Apr 22, 2018

Latest commit would set it to 20. Also catches any server errors coming out of the hi parameter.

Abe/abe.py Outdated
hi = get_int_param(page, 'hi')
orig_hi = hi
except ValueError:
hi = 19

This comment has been minimized.

@dermoth

dermoth Apr 22, 2018

Member

Set to none here, let Abe find the tip

@dermoth

This comment has been minimized.

Member

dermoth commented Apr 22, 2018

My question was simple... what happens right now with a negative count? Shows 0 records or Exception? I'd like to know.

Regarding the new patch, I think hi should be set to None on ValueError, i.e. default behavior with no param.

Thanks

@Lvl4Sword

This comment has been minimized.

Contributor

Lvl4Sword commented Apr 22, 2018

Negative count would give the error:
A server error occurred. Please contact the administrator.
I'll fix the hi parameter, one moment..

@Lvl4Sword

This comment has been minimized.

Contributor

Lvl4Sword commented Apr 22, 2018

That should work now 👍

Abe/abe.py Outdated
try:
hi = get_int_param(page, 'hi')
orig_hi = hi

This comment has been minimized.

@dermoth

dermoth Apr 22, 2018

Member

I should have paid more attention on my first review... that orig_hi should go after the try block - looking at the original core it was always set to the same value as hi but now won't exist after exception handling.

orig_hi = hi outside of try/except.
Looking closer at this, it looks like orig_hi is only used in two places and could be removed completely.
After the first "if hi is None", orig_hi wouldn't inherit the new version of hi, so it would still be None if a ValueError exception was raised.

@dermoth dermoth changed the title from Catch negative count to Catch invalid query params Apr 22, 2018

@dermoth dermoth merged commit d33f6e8 into bitcoin-abe:master Apr 22, 2018

@dermoth

This comment has been minimized.

Member

dermoth commented Apr 22, 2018

I squashed your commits as there were too many back and forth changes, wouldn't make much sense to someone else reviewing the commits and the squashed diff is quite straightforward.

This means you'll probably have to drop your patch-1 as it diverged now.

Thanks

@Lvl4Sword Lvl4Sword deleted the Lvl4Sword:patch-1 branch Apr 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment