package(mongo) Do not break API to handle validation errors with internal errors #999
Conversation
I don't understand, currently we can' have a validation method which returns something else than a My attempt here is to open to this possibility because indeed there are some simtuations in which a validation may fail because we can't access some data, or something internal is failing, with this PR it becomes possible without breaking the API. I thought it would be quite a elegant way to solve the initial problem which is indeed a problem. |
|
How would you handle this without breaking the API? |
There was a problem hiding this comment.
Sorry, I didn't understood correctly at the first read.
I'm already approving despite the suggestions and the failing CI. I let you choose whether you want to take into account the suggestions or not.
After releasing this, I think it would be nice to send a message on team-tech so that all Go developers are aware of this change.
| // Validate will be used if no ValidateWithInternalError is defined on a document | ||
| // It is not useful to have both defined on a document, only ValidationWithInternalError | ||
| // would be used in this case | ||
| Validate(ctx context.Context) *errors.ValidationErrors |
There was a problem hiding this comment.
question: do we want to deprecate this method? In such case, we should document it IMO:
| Validate(ctx context.Context) *errors.ValidationErrors | |
| // Deprecated: ... | |
| Validate(ctx context.Context) *errors.ValidationErrors |
There was a problem hiding this comment.
I don't think we want to deprecate, it's just 2 ways to define validation, I think that keeping a simple signature for the comon case works well.
mongo/document/document.go
Outdated
| if err == ErrValidateNoInternalErrorFunc { | ||
| validationErrors = doc.Validate(ctx) | ||
| } else if err != nil { | ||
| return errors.Wrap(ctx, err, "fail to validate document") |
There was a problem hiding this comment.
nitpick(non-blocking):
| return errors.Wrap(ctx, err, "fail to validate document") | |
| return errors.Wrap(ctx, err, "validate document") |
mongo/document/document.go
Outdated
| "collection": collectionName, | ||
| "doc_id": doc.getID().Hex(), |
There was a problem hiding this comment.
suggestion: what do you think about adding these fields in the logger in the save function?
I'll take t hem all into consideration :) |
…rnal errors Fixes #998
leo-scalingo
force-pushed
the
fix/998/validation-internal-errors
branch
from
25833f8 to
e800254
Compare
leo-scalingo
force-pushed
the
fix/998/validation-internal-errors
branch
from
3d3595f to
ae3124d
Compare
Fixes #998
CHANGELOG.md