libbeat: schema.Apply() returns plain error or nil #7167
Conversation
schema.Apply() returned an schema.Errors object as error, with this object is not easy to check if there was really an error, specially when optional fields are involved. This leads to lots of cases where these errors are being ignored. This change makes Apply to return an error only if a non-optional field is missing or in any case if some field is misformatted. Otherwise it returns plain nil, so idiomatic go error checking can be used.
jsoriano
added
in progress
| @@ -55,7 +50,7 @@ func (conv Conv) HasKey(key string) bool { | |||
| // implements Mapper interface for structure | |||
| type Object map[string]Mapper | |||
|
|
|||
| func (o Object) Map(key string, event common.MapStr, data map[string]interface{}) *Errors { | |||
| func (o Object) Map(key string, event common.MapStr, data map[string]interface{}) Errors { | |||
There was a problem hiding this comment.
exported method Object.Map should have comment or be unexported
|
|
||
| func (errs *Errors) HasRequiredErrors() bool { | ||
| for _, err := range *errs { | ||
| func (errs Errors) HasRequiredErrors() bool { |
There was a problem hiding this comment.
exported method Errors.HasRequiredErrors should have comment or be unexported
| @@ -6,50 +6,35 @@ import ( | |||
| "github.com/elastic/beats/libbeat/logp" | |||
| ) | |||
|
|
|||
| type Errors []Error | |||
| type Errors []*Error | |||
There was a problem hiding this comment.
exported type Errors should have comment or be unexported
| @@ -34,3 +34,16 @@ func (err *Error) IsType(errorType ErrorType) bool { | |||
| func (err *Error) Error() string { | |||
| return fmt.Sprintf("Missing field: %s, Error: %s", err.key, err.message) | |||
| } | |||
|
|
|||
| type KeyNotFoundError struct { | |||
There was a problem hiding this comment.
exported type KeyNotFoundError should have comment or be unexported
| @@ -55,7 +50,7 @@ func (conv Conv) HasKey(key string) bool { | |||
| // implements Mapper interface for structure | |||
| type Object map[string]Mapper | |||
|
|
|||
| func (o Object) Map(key string, event common.MapStr, data map[string]interface{}) *Errors { | |||
| func (o Object) Map(key string, event common.MapStr, data map[string]interface{}) Errors { | |||
There was a problem hiding this comment.
exported method Object.Map should have comment or be unexported
|
|
||
| func (errs *Errors) HasRequiredErrors() bool { | ||
| for _, err := range *errs { | ||
| func (errs Errors) HasRequiredErrors() bool { |
There was a problem hiding this comment.
exported method Errors.HasRequiredErrors should have comment or be unexported
| @@ -6,50 +6,35 @@ import ( | |||
| "github.com/elastic/beats/libbeat/logp" | |||
| ) | |||
|
|
|||
| type Errors []Error | |||
| type Errors []*Error | |||
There was a problem hiding this comment.
exported type Errors should have comment or be unexported
| @@ -34,3 +34,16 @@ func (err *Error) IsType(errorType ErrorType) bool { | |||
| func (err *Error) Error() string { | |||
| return fmt.Sprintf("Missing field: %s, Error: %s", err.key, err.message) | |||
| } | |||
|
|
|||
| type KeyNotFoundError struct { | |||
There was a problem hiding this comment.
exported type KeyNotFoundError should have comment or be unexported
|
WIP, I'm not completely happy with the result and probably I'm missing some cases. |
|
For testing purpose it is useful to also check for non required fields if they are missing. Is this still possible? |
|
Not with the returned error. For testing we might think in other approximations for explicitly checking that optional fields are also present, but I wouldn't make it return an error by default if an optional field is missing, this breaks a bit the nature of a non-required thing. |
|
I agree with the statement above. The part that I worry is that our current testing in some cases relies on checking for the error, converting it and then check if there are also no optional fields missing. We can definitively decide to move forward with this suggestion and fix this as a follow up. |
|
I'm following with this with other approach (in #7335), returning a "result" object instead of just an error, this result object would make easier to check the errors, and also the list of missing fields, what could be used in tests. |
|
Closing this one, continuing in #7335 |
schema.Apply() returned an schema.Errors object as error, with this
object is not easy to check if there was really an error, specially when
optional fields are involved. This leads to lots of cases where these
errors are being ignored.
This change makes Apply to return an error only if a non-optional field
is missing or in any case if some field is misformatted. Otherwise it
returns plain nil, so a more idiomatic go error checking can be used.