Don't 500 if search attribute is invalid #925
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| %% key, typically trying to access a value as a hash that is actually | ||
| %% a string (i.e. uptime.seconds) | ||
| error:function_clause -> null; | ||
| Error:Reason -> {Error, Reason} |
There was a problem hiding this comment.
If the term that comes back can't be converted as ejson -> json, we'll still end up with a 500 as we prepare the body response. Maybe a generic error string for the return value in that case - or just 'null' - , and use error_loggerto capture the details.
tduffield
force-pushed
the
SPOOL-319/dont-500-on-invalid-attribute
branch
from
c19cb94 to
c029b86
Compare
|
👍 |
In the case where a user, when providing a search, provides an attribute constraint (-a flag) that is invalid, we currently throw a 500. An invalid attribute constraint most commonly is specifying a nested hash using dot-notation when one or more values in that constraint are, in fact, not a hash. This change prevents the system from throwing a 500 and presents the user with a message indicating that the attribute does not exist. For example, `knife search node "*:*" -a uptime.seconds` is invalid because the key `'uptime'` does exist but it is a string, not a hash. Thus, ej attempts to fetch the key `'seconds'` from that string, throwing an error within ej for an invalid function clause. With this change, the search will return "ERROR - Attribute does not exist". Signed-off-by: Tom Duffield <[email protected]>
marcparadise
force-pushed
the
SPOOL-319/dont-500-on-invalid-attribute
branch
from
c029b86 to
cecf0fa
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the case where a user, when providing a search, provides an attribute
constraint (-a flag) that is invalid, we currently throw a 500. An
invalid attribute constraint most commonly is specifying a nested hash
using dot-notation when one or more values in that constraint are, in
fact, not a hash. This change prevents the system from throwing a 500
and presents the user with a more correct result.
For example,
knife search node "*:*" -a uptime.secondsis invalidbecause the key
'uptime'does exist but it is a string, not ahash. Thus, ej attempts to fetch the key
'seconds'from that string,throwing an error within ej for an invalid function clause. With this change,
we simply return "ERROR - Attribute does not exist".
The reason there are no tests is that there isn't a great way to test this code path as it is a private function nested inside several helpers. The time it would take to setup mocks to exercise this code path likely isn't worth the time given how easily this is to replicate manually.