From 4a2938b19137c07eba56d92c19dc0d5daff04ea1 Mon Sep 17 00:00:00 2001 From: jf-tech Date: Sun, 27 Sep 2020 14:22:22 +1300 Subject: [PATCH 1/3] Fix omniv2 custom FileFormat extensibility problem Currently if a custom `FileFormat` is given in `HandlerParams` to omniv2 handler creation call, this `FileFormat` becomes the only FileFormat for the omni handler. This behavior would make it difficult to deal with situation where we have to ingest many types of inputs using omni schema. (Saying it being difficult not impossible is because we can theoretically use 2 Extensions, each of which uses the same omni handler, but the first one with custom `FileFormat` and second with nil, thus the builtin `FileFormat`s will kick in. Possible, but incredibly ugly.) Instead, omni handler's `HandlerParams` should allow a list of custom `FileFormat`s. During schema probing, we will combine them with the builtin `FileFormat`s, with priority given the custom ones. If one `FileFormat` accepts the input then it will be used during ingestion; if not, omni handler will keep on probing the rest of the `FileFormat`s. --- handlers/omni/v2/handler.go | 17 +++++---- handlers/omni/v2/handler_test.go | 37 ++++++++++--------- .../customfileformats/jsonlog/sample_test.go | 5 ++- 3 files changed, 33 insertions(+), 26 deletions(-) diff --git a/handlers/omni/v2/handler.go b/handlers/omni/v2/handler.go index 4b6425a..d4812ec 100644 --- a/handlers/omni/v2/handler.go +++ b/handlers/omni/v2/handler.go @@ -20,8 +20,8 @@ const ( // HandlerParams allows user of omniparser to provide omniv2 schema handler customization. type HandlerParams struct { - CustomFileFormat omniv2fileformat.FileFormat - CustomParseFuncs transform.CustomParseFuncs + CustomFileFormats []omniv2fileformat.FileFormat + CustomParseFuncs transform.CustomParseFuncs } // CreateHandler parses, validates and creates an omni-schema based handler. @@ -35,7 +35,8 @@ func CreateHandler(ctx *handlers.HandlerCtx) (handlers.SchemaHandler, error) { // err is already context formatted. return nil, err } - finalOutputDecl, err := transform.ValidateTransformDeclarations(ctx.Content, ctx.CustomFuncs, customParseFuncs(ctx)) + finalOutputDecl, err := transform.ValidateTransformDeclarations( + ctx.Content, ctx.CustomFuncs, customParseFuncs(ctx)) if err != nil { return nil, fmt.Errorf( "schema '%s' 'transform_declarations' validation failed: %s", @@ -75,15 +76,15 @@ func customParseFuncs(ctx *handlers.HandlerCtx) transform.CustomParseFuncs { } func fileFormats(ctx *handlers.HandlerCtx) []omniv2fileformat.FileFormat { - // If caller specifies a custom FileFormat, we'll use it (and it only); - // otherwise we'll use the builtin ones. formats := []omniv2fileformat.FileFormat{ omniv2json.NewJSONFileFormat(ctx.Name), omniv2xml.NewXMLFileFormat(ctx.Name), - // TODO more bulit-in omniv2 file formats to come. + // TODO more built-in omniv2 file formats to come. } - if ctx.HandlerParams != nil && ctx.HandlerParams.(*HandlerParams).CustomFileFormat != nil { - formats = []omniv2fileformat.FileFormat{ctx.HandlerParams.(*HandlerParams).CustomFileFormat} + if ctx.HandlerParams != nil { + // If caller specifies list custom FileFormats, we'll give them priority + // over builtin ones. + formats = append(ctx.HandlerParams.(*HandlerParams).CustomFileFormats, formats...) } return formats } diff --git a/handlers/omni/v2/handler_test.go b/handlers/omni/v2/handler_test.go index 4fd668b..1c8dee6 100644 --- a/handlers/omni/v2/handler_test.go +++ b/handlers/omni/v2/handler_test.go @@ -14,6 +14,7 @@ import ( "github.com/jf-tech/omniparser/errs" "github.com/jf-tech/omniparser/handlers" omniv2fileformat "github.com/jf-tech/omniparser/handlers/omni/v2/fileformat" + omniv2json "github.com/jf-tech/omniparser/handlers/omni/v2/fileformat/json" "github.com/jf-tech/omniparser/handlers/omni/v2/transform" "github.com/jf-tech/omniparser/header" "github.com/jf-tech/omniparser/transformctx" @@ -127,24 +128,27 @@ func TestCreateHandler_TransformDeclarationsInCodeValidationFailed(t *testing.T) assert.Nil(t, p) } -func TestCreateHandler_CustomFileFormat_FormatNotSupported(t *testing.T) { +func TestCreateHandler_CustomFileFormat_FormatNotSupported_Fallback(t *testing.T) { p, err := CreateHandler( &handlers.HandlerCtx{ Header: header.Header{ ParserSettings: header.ParserSettings{ - Version: version, + Version: version, + FileFormatType: "json", }, }, - Content: []byte(`{"transform_declarations": { "FINAL_OUTPUT": {} }}`), + Content: []byte(`{"transform_declarations": { "FINAL_OUTPUT": { "xpath": "." }}}`), HandlerParams: &HandlerParams{ - CustomFileFormat: testFileFormat{ - validateSchemaErr: errs.ErrSchemaNotSupported, + CustomFileFormats: []omniv2fileformat.FileFormat{ + // Having the custom FileFormat returns ErrSchemaNotSupported + // causes it to fallback and continue probing the built-in FileFormats. + testFileFormat{validateSchemaErr: errs.ErrSchemaNotSupported}, }, }, }) - assert.Error(t, err) - assert.Equal(t, "schema not supported", err.Error()) - assert.Nil(t, p) + assert.NoError(t, err) + assert.IsType(t, omniv2json.NewJSONFileFormat(""), p.(*schemaHandler).fileFormat) + assert.Equal(t, ".", p.(*schemaHandler).formatRuntime.(string)) } func TestCreateHandler_CustomFileFormat_ValidationFailure(t *testing.T) { @@ -157,8 +161,8 @@ func TestCreateHandler_CustomFileFormat_ValidationFailure(t *testing.T) { }, Content: []byte(`{"transform_declarations": { "FINAL_OUTPUT": {} }}`), HandlerParams: &HandlerParams{ - CustomFileFormat: testFileFormat{ - validateSchemaErr: errors.New("validation failure"), + CustomFileFormats: []omniv2fileformat.FileFormat{ + testFileFormat{validateSchemaErr: errors.New("validation failure")}, }, }, }) @@ -177,15 +181,14 @@ func TestCreateHandler_CustomFileFormat_Success(t *testing.T) { }, Content: []byte(`{"transform_declarations": { "FINAL_OUTPUT": {} }}`), HandlerParams: &HandlerParams{ - CustomFileFormat: testFileFormat{ - validateSchemaRuntime: "runtime data", + CustomFileFormats: []omniv2fileformat.FileFormat{ + testFileFormat{validateSchemaRuntime: "runtime data"}, }, }, }) assert.NoError(t, err) - plugin := p.(*schemaHandler) - assert.Equal(t, "runtime data", plugin.fileFormat.(testFileFormat).validateSchemaRuntime.(string)) - assert.Equal(t, "runtime data", plugin.formatRuntime.(string)) + assert.IsType(t, &schemaHandler{}, p) + assert.Equal(t, "runtime data", p.(*schemaHandler).formatRuntime.(string)) } func TestCreateHandler_CustomParseFuncs_Success(t *testing.T) { @@ -226,7 +229,7 @@ func TestNewIngester_CustomFileFormat_Failure(t *testing.T) { } func TestNewIngester_CustomFileFormat_Success(t *testing.T) { - plugin := &schemaHandler{ + handler := &schemaHandler{ ctx: &handlers.HandlerCtx{ CustomFuncs: customfuncs.BuiltinCustomFuncs, }, @@ -234,7 +237,7 @@ func TestNewIngester_CustomFileFormat_Success(t *testing.T) { formatRuntime: "test runtime", } ctx := &transformctx.Ctx{InputName: "test-input"} - ip, err := plugin.NewIngester(ctx, strings.NewReader("test input")) + ip, err := handler.NewIngester(ctx, strings.NewReader("test input")) assert.NoError(t, err) g := ip.(*ingester) assert.Equal(t, ctx, g.ctx) diff --git a/samples/omniv2/customfileformats/jsonlog/sample_test.go b/samples/omniv2/customfileformats/jsonlog/sample_test.go index 7e74f82..3a281c5 100644 --- a/samples/omniv2/customfileformats/jsonlog/sample_test.go +++ b/samples/omniv2/customfileformats/jsonlog/sample_test.go @@ -13,6 +13,7 @@ import ( "github.com/jf-tech/omniparser" "github.com/jf-tech/omniparser/customfuncs" omniv2 "github.com/jf-tech/omniparser/handlers/omni/v2" + omniv2fileformat "github.com/jf-tech/omniparser/handlers/omni/v2/fileformat" "github.com/jf-tech/omniparser/samples/omniv2/customfileformats/jsonlog/jsonlogformat" "github.com/jf-tech/omniparser/transformctx" ) @@ -58,7 +59,9 @@ func TestSample(t *testing.T) { CreateHandler: omniv2.CreateHandler, // Use the same omniv2 handler HandlerParams: &omniv2.HandlerParams{ // But use our own FileFormat. - CustomFileFormat: jsonlogformat.NewJSONLogFileFormat(schemaFileBaseName), + CustomFileFormats: []omniv2fileformat.FileFormat{ + jsonlogformat.NewJSONLogFileFormat(schemaFileBaseName), + }, }, CustomFuncs: customfuncs.CustomFuncs{ "normalize_severity": normalizeSeverity, From 6e71a1b03a072551317d5abb1066604a3036721c Mon Sep 17 00:00:00 2001 From: jf-tech Date: Sun, 27 Sep 2020 14:35:50 +1300 Subject: [PATCH 2/3] fix comments --- handlers/omni/v2/handler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/handlers/omni/v2/handler.go b/handlers/omni/v2/handler.go index d4812ec..4a9cb08 100644 --- a/handlers/omni/v2/handler.go +++ b/handlers/omni/v2/handler.go @@ -82,7 +82,7 @@ func fileFormats(ctx *handlers.HandlerCtx) []omniv2fileformat.FileFormat { // TODO more built-in omniv2 file formats to come. } if ctx.HandlerParams != nil { - // If caller specifies list custom FileFormats, we'll give them priority + // If caller specifies a list of custom FileFormats, we'll give them priority // over builtin ones. formats = append(ctx.HandlerParams.(*HandlerParams).CustomFileFormats, formats...) } From 53c7803dfaca7f9991c4ed329f0ac833187e9e74 Mon Sep 17 00:00:00 2001 From: jf-tech Date: Sun, 27 Sep 2020 14:58:50 +1300 Subject: [PATCH 3/3] Simon's comment: what if HandlerParams type is incorrect --- handlers/omni/v2/handler.go | 19 +++++++++++++------ handlers/omni/v2/handler_test.go | 17 +++++++++++++++++ 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/handlers/omni/v2/handler.go b/handlers/omni/v2/handler.go index 4a9cb08..f2bf652 100644 --- a/handlers/omni/v2/handler.go +++ b/handlers/omni/v2/handler.go @@ -68,7 +68,10 @@ func customParseFuncs(ctx *handlers.HandlerCtx) transform.CustomParseFuncs { if ctx.HandlerParams == nil { return nil } - params := ctx.HandlerParams.(*HandlerParams) + params, ok := ctx.HandlerParams.(*HandlerParams) + if !ok { + return nil + } if len(params.CustomParseFuncs) == 0 { return nil } @@ -81,12 +84,16 @@ func fileFormats(ctx *handlers.HandlerCtx) []omniv2fileformat.FileFormat { omniv2xml.NewXMLFileFormat(ctx.Name), // TODO more built-in omniv2 file formats to come. } - if ctx.HandlerParams != nil { - // If caller specifies a list of custom FileFormats, we'll give them priority - // over builtin ones. - formats = append(ctx.HandlerParams.(*HandlerParams).CustomFileFormats, formats...) + if ctx.HandlerParams == nil { + return formats + } + params, ok := ctx.HandlerParams.(*HandlerParams) + if !ok { + return formats } - return formats + // If caller specifies a list of custom FileFormats, we'll give them priority + // over builtin ones. + return append(params.CustomFileFormats, formats...) } type schemaHandler struct { diff --git a/handlers/omni/v2/handler_test.go b/handlers/omni/v2/handler_test.go index 1c8dee6..0e01e57 100644 --- a/handlers/omni/v2/handler_test.go +++ b/handlers/omni/v2/handler_test.go @@ -128,6 +128,23 @@ func TestCreateHandler_TransformDeclarationsInCodeValidationFailed(t *testing.T) assert.Nil(t, p) } +func TestCreateHandler_HandlerParamsTypeNotRight_Fallback(t *testing.T) { + p, err := CreateHandler( + &handlers.HandlerCtx{ + Header: header.Header{ + ParserSettings: header.ParserSettings{ + Version: version, + FileFormatType: "json", + }, + }, + Content: []byte(`{"transform_declarations": { "FINAL_OUTPUT": { "xpath": "." }}}`), + HandlerParams: "not nil but not the right type", + }) + assert.NoError(t, err) + assert.IsType(t, omniv2json.NewJSONFileFormat(""), p.(*schemaHandler).fileFormat) + assert.Equal(t, ".", p.(*schemaHandler).formatRuntime.(string)) +} + func TestCreateHandler_CustomFileFormat_FormatNotSupported_Fallback(t *testing.T) { p, err := CreateHandler( &handlers.HandlerCtx{