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

Best effort validation of user input #29

Closed
wants to merge 3 commits into from

Conversation

zstyblik
Copy link

@zstyblik zstyblik commented Jan 18, 2018

PR adds best effort validation of user input. Fall back to sane defaults in case of invalid input.

Fixes #22

Unfortunately, I haven't found a better way. Having said that, it's
entrirely possible this is going to fall short.
Commit adds best effort validation of user input. Should the input be
invalid, defaults will be used instead for sanity.
@zstyblik
Copy link
Author

First of all and as before, I'm no Java programmer. Second, I haven't found a better way to validate user input than to do it this way, resp. via checkUrl call or whatever it is. There is a drawback, though. Yes, message pops up and warns the user, but it doesn't prevent user from submitting invalid input. That's where third thing comes in - defaults. I think it's better than to throw an exception. Though, I perceived the code to be ugly.

@jovandeginste, please, can you give this a look?

@zstyblik zstyblik changed the title Best effort validation of user input [WIP] Best effort validation of user input Jan 22, 2018
Commit refactors input validation in constructor. Logic has been moved
into respective functions and code has been simplified.
Commit also adds unit tests to prove code in question works as expected.
@zstyblik zstyblik changed the title [WIP] Best effort validation of user input Best effort validation of user input Jan 22, 2018
@zstyblik
Copy link
Author

I have refactored the code a bit and added unit tests. Unfortunately, I have no idea how to write unit tests for form validation part.

@zstyblik
Copy link
Author

... or @jippi ?

@jovandeginste
Copy link

@zstyblik Due to personal stuff, I'm taking some slow weeks.

As a default, I would rather have the code throw errors than do something I didn't expect it to do.

@zstyblik
Copy link
Author

zstyblik commented Jan 27, 2018

@jovandeginste sure, no problem.

As a default, I would rather have the code throw errors than do something I didn't expect it to do.

I'm open to suggestions. However, Java exception says pretty much nothing to the user like any exception from any programming language and code you're not familiar with. In other words, I really hate when Jenkins goes exception on me for no obvious reason.
Anyway, give this patch a try and you will see. Also, as I've stated above, I haven't found way how to prevent form being submitted.

EDIT: Let me rephrase it a bit. I'm talking about communicating the error to the user. Defaults are used as the last line of defense when form validation errors are ignored and form is submitted regardless. If it's possible to raise exception which is easy to understand by the user, let's do that.

@phedoreanu phedoreanu force-pushed the master branch 2 times, most recently from a326e2d to 19e8175 Compare April 8, 2019 09:11
@phedoreanu
Copy link

@zstyblik Would you please rebase and add the security fixes from #42 ?

@phedoreanu phedoreanu closed this Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exception raised when no datacenter is given
3 participants