-
Notifications
You must be signed in to change notification settings - Fork 34
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
Improve error handling by exposing exceptions instead of silently consuming them #45
Improve error handling by exposing exceptions instead of silently consuming them #45
Conversation
* Make exceptions available via unused `validationErrors` field * Improve code readability.
if (rules.isEmpty()) return emptyList() | ||
|
||
val dataJsonNode = prepareData(externalParameter, payload) | ||
val hcertVersion: Triple<Int, Int, Int>? = hcertVersionString.toVersion() | ||
|
||
return rules.map { | ||
checkRule( | ||
rule = it, | ||
dataJsonNode = dataJsonNode, | ||
hcertVersion = hcertVersion, | ||
certificateType = certificateType | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Early return to prervent unnecessary nesting, then treat this as data preperation step.
val isCompatibleVersion = rule.engine == CERTLOGIC_KEY && | ||
ruleEngineVersion != null && | ||
CERTLOGIC_VERSION.isGreaterOrEqualThan(ruleEngineVersion) && | ||
hcertVersion != null && | ||
schemaVersion != null && | ||
hcertVersion.first == schemaVersion.first && | ||
hcertVersion.isGreaterOrEqualThan(schemaVersion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explicit version compatibility check.
val res = if (isCompatibleVersion) { | ||
try { | ||
when (jsonLogicValidator.isDataValid(rule.logic, dataJsonNode)) { | ||
true -> Result.PASSED | ||
false -> Result.FAIL | ||
} | ||
val cur: String = affectedFieldsDataRetriever.getAffectedFieldsData( | ||
rule, | ||
dataJsonNode, | ||
certificateType | ||
) | ||
validationResults.add( | ||
ValidationResult( | ||
rule, | ||
res, | ||
cur, | ||
null | ||
) | ||
) | ||
} catch (e: Exception) { | ||
validationErrors.add(e) | ||
Result.OPEN | ||
} | ||
validationResults | ||
} else { | ||
emptyList() | ||
Result.OPEN | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still map exceptions to OPEN
, but store the Exception
for the result.
Exception
instead of Throwable
to not catch Error
.
https://stackoverflow.com/questions/6083248/is-it-a-bad-practice-to-catch-throwable
rule, | ||
res, | ||
cur, | ||
if (validationErrors.isEmpty()) null else validationErrors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could also just stay an empty list, but I didn't want to alter the behavior to the outside, e.g. if a library consumer checks for null
.
Thank you for the update! All comments definitely make sense |
This is a follow up to #44
Debugging was really difficult because there was no visible error.
ValidationResult
already has a field to return errors that we can use, it was just set tonull
.This PR:
Exception
notThrowable
as the libraries should not quietly contain anError
being thrown.Exception
map it toOPEN
but save the exception and fill the validation result with any errors, otherwise set it to null.The library should behave exactly the same to the outside, except that if there is an internal
Exception
being thrown, thenValidationResult.validationErrors
is notnull
.