From dbc1d30fd9cc8cc3b62c580ba6dae2e13267e5bb Mon Sep 17 00:00:00 2001 From: Grzegorz Piwowarek Date: Sun, 14 Jul 2024 22:07:23 +0200 Subject: [PATCH] Don't expose internal '::' separators to clients (#524) Double colons are our internal implementation detail - described [here](https://github.com/QuesmaOrg/quesma/blob/399fd231bb97437687bc18b88a734939fe47357e/adr/4_double_colon_as_nested_structure_separator.md). Users should not know about it, but use classic dots as separators. depends on: - https://github.com/QuesmaOrg/quesma/pull/526 - https://github.com/QuesmaOrg/quesma/pull/525 - https://github.com/QuesmaOrg/quesma/pull/527 --- .../quesma/config/local-dev.yaml | 3 +- http_requests/field_caps.http | 2 +- .../elastic_clickhouse_fields.go | 13 +------ quesma/plugins/registry/plugin_registry.go | 7 +--- quesma/queryparser/query_parser.go | 36 ++++++++----------- quesma/schema/registry.go | 26 ++++++++++---- 6 files changed, 40 insertions(+), 47 deletions(-) diff --git a/examples/kibana-sample-data/quesma/config/local-dev.yaml b/examples/kibana-sample-data/quesma/config/local-dev.yaml index 25960d189..5f102d51a 100644 --- a/examples/kibana-sample-data/quesma/config/local-dev.yaml +++ b/examples/kibana-sample-data/quesma/config/local-dev.yaml @@ -26,6 +26,7 @@ indexes: products.product_name: "text" geoip.location: "geo_point" category: "text" + manufacturer: "text" kibana_sample_data_flights: enabled: true mappings: @@ -36,7 +37,7 @@ indexes: mappings: ip: "ip" clientip: "ip" - geo::coordinates: "point" + geo.coordinates: "point" extension: "text" url: "text" aliases: diff --git a/http_requests/field_caps.http b/http_requests/field_caps.http index 26d70e274..73904c432 100644 --- a/http_requests/field_caps.http +++ b/http_requests/field_caps.http @@ -1,4 +1,4 @@ -POST http://localhost:8080/kibana_sample_data_flights/_field_caps +POST http://localhost:9200/kibana_sample_data_flights/_field_caps Content-Type: application/json { diff --git a/quesma/plugins/elastic_clickhouse_fields/elastic_clickhouse_fields.go b/quesma/plugins/elastic_clickhouse_fields/elastic_clickhouse_fields.go index 5cffabc73..9b2b2d7da 100644 --- a/quesma/plugins/elastic_clickhouse_fields/elastic_clickhouse_fields.go +++ b/quesma/plugins/elastic_clickhouse_fields/elastic_clickhouse_fields.go @@ -24,10 +24,6 @@ func doubleColons2dot(input string) string { return strings.ReplaceAll(input, doubleColons, dot) } -func dot2DoubleColons(input string) string { - return strings.ReplaceAll(input, dot, doubleColons) -} - func dot2SQLNative(input string) string { return strings.ReplaceAll(input, dot, sqlNative) } @@ -259,8 +255,7 @@ func (p *Dot2DoubleColons) GetTableColumnFormatter(table string, cfg config.Ques type Dot2DoubleColons2Dot struct{} func (*Dot2DoubleColons2Dot) matches(table string) bool { - // this is enabled for e-commerce data, it makes dashboard work - return strings.HasPrefix(table, "kibana_sample_data_ecommerce") + return true } func (*Dot2DoubleColons2Dot) IngestTransformer() plugins.IngestTransformer { @@ -282,9 +277,6 @@ func (p *Dot2DoubleColons2Dot) GetTableColumnFormatter(table string, cfg config. } func (p *Dot2DoubleColons2Dot) ApplyQueryTransformers(table string, cfg config.QuesmaConfiguration, transformers []plugins.QueryTransformer) []plugins.QueryTransformer { - if p.matches(table) { - transformers = append(transformers, &queryTransformer{translate: dot2DoubleColons}) - } return transformers } @@ -296,9 +288,6 @@ func (p *Dot2DoubleColons2Dot) ApplyResultTransformers(table string, cfg config. } func (p *Dot2DoubleColons2Dot) ApplyFieldCapsTransformers(table string, cfg config.QuesmaConfiguration, transformers []plugins.FieldCapsTransformer) []plugins.FieldCapsTransformer { - if p.matches(table) { - transformers = append(transformers, &fieldCapsTransformer{translate: doubleColons2dot}) - } return transformers } diff --git a/quesma/plugins/registry/plugin_registry.go b/quesma/plugins/registry/plugin_registry.go index 8709c882e..60803e9c1 100644 --- a/quesma/plugins/registry/plugin_registry.go +++ b/quesma/plugins/registry/plugin_registry.go @@ -12,12 +12,7 @@ import ( var registeredPlugins []plugins.Plugin func init() { - registeredPlugins = []plugins.Plugin{ - // TODO below plugins are disabled due to some - // interferences with other components - &elastic_clickhouse_fields.Dot2DoubleColons2Dot{}, - //&elastic_clickhouse_fields.Dot2DoubleUnderscores2Dot{}, - &elastic_clickhouse_fields.Dot2DoubleColons{}} + registeredPlugins = []plugins.Plugin{&elastic_clickhouse_fields.Dot2DoubleColons2Dot{}} } func QueryTransformerFor(table string, cfg config.QuesmaConfiguration) plugins.QueryTransformer { diff --git a/quesma/queryparser/query_parser.go b/quesma/queryparser/query_parser.go index 666f52dd2..1daec77e1 100644 --- a/quesma/queryparser/query_parser.go +++ b/quesma/queryparser/query_parser.go @@ -931,12 +931,9 @@ func (cw *ClickhouseQueryTranslator) parseExists(queryMap QueryMap) model.Simple ), "=", model.NewLiteral("0")) case clickhouse.NotExists: // TODO this is a workaround for the case when the field is a point - schemaInstance, exists := cw.SchemaRegistry.FindSchema(schema.TableName(cw.Table.Name)) - if exists { - if value, ok := schemaInstance.Fields[schema.FieldName(fieldName)]; ok { - if value.Type.Equal(schema.TypePoint) { - return model.NewSimpleQuery(sql, true) - } + if schemaInstance, exists := cw.SchemaRegistry.FindSchema(schema.TableName(cw.Table.Name)); exists { + if value, ok := schemaInstance.ResolveFieldByInternalName(fieldName); ok && value.Type.Equal(schema.TypePoint) { + return model.NewSimpleQuery(sql, true) } } attrs := cw.Table.GetAttributesList() @@ -1345,33 +1342,30 @@ func createSortColumn(fieldName, ordering string) (model.OrderByExpr, error) { // What prevents us from moving it to transformation pipeline now, is that // we need to anotate this field somehow in the AST, to be able // to distinguish it from other fields -func (cw *ClickhouseQueryTranslator) ResolveField(ctx context.Context, fieldName string) (field string) { +func (cw *ClickhouseQueryTranslator) ResolveField(ctx context.Context, fieldName string) string { // Alias resolution should occur *after* the query is parsed, not during the parsing if cw.SchemaRegistry == nil { logger.Error().Msg("Schema registry is not set") - field = fieldName - return + return fieldName } schemaInstance, exists := cw.SchemaRegistry.FindSchema(schema.TableName(cw.Table.Name)) if !exists { logger.Error().Msgf("Schema for table [%s] not found, this should never happen", cw.Table.Name) - field = fieldName - return + return fieldName } + // TODO this should be handled eventually at schema level + fieldName = strings.TrimSuffix(fieldName, ".keyword") + fieldName = strings.TrimSuffix(fieldName, ".text") + if resolvedField, ok := schemaInstance.ResolveField(fieldName); ok { - field = resolvedField.InternalPropertyName.AsString() + return resolvedField.InternalPropertyName.AsString() } else { - // fallback to original field name - logger.DebugWithCtx(ctx).Msgf("field '%s' referenced, but not found in schema", fieldName) - field = fieldName - } - - if field != "*" && field != "_all" && field != "_doc" && field != "_id" && field != "_index" { - if _, ok := schemaInstance.Fields[schema.FieldName(field)]; !ok { - logger.DebugWithCtx(ctx).Msgf("field '%s' referenced, but not found in schema", fieldName) + if fieldName != "*" && fieldName != "_all" && fieldName != "_doc" && fieldName != "_id" && fieldName != "_index" { + logger.DebugWithCtx(ctx).Msgf("field '%s' referenced, but not found in schema, falling back to original name", fieldName) } + + return fieldName } - return } func (cw *ClickhouseQueryTranslator) parseSizeExists(queryMap QueryMap) (size int, ok bool) { sizeRaw, exists := queryMap["size"] diff --git a/quesma/schema/registry.go b/quesma/schema/registry.go index 38f05a6ce..08dcb3653 100644 --- a/quesma/schema/registry.go +++ b/quesma/schema/registry.go @@ -5,6 +5,7 @@ package schema import ( "quesma/logger" "quesma/quesma/config" + "strings" ) type ( @@ -84,7 +85,12 @@ func (s *schemaRegistry) populateSchemaFromStaticConfiguration(indexConfiguratio if deprecatedConfigInUse(indexConfiguration) { for fieldName, fieldType := range indexConfiguration.TypeMappings { if resolvedType, valid := ParseType(fieldType); valid { - fields[FieldName(fieldName)] = Field{PropertyName: FieldName(fieldName), InternalPropertyName: FieldName(fieldName), Type: resolvedType} + if resolvedType.Equal(TypePoint) { + // TODO replace with notion of ephemeral types + fields[FieldName(fieldName)] = Field{PropertyName: FieldName(fieldName), InternalPropertyName: FieldName(strings.Replace(fieldName, ".", "::", -1)), Type: resolvedType} + } else { + fields[FieldName(fieldName)] = Field{PropertyName: FieldName(fieldName), InternalPropertyName: FieldName(fieldName), Type: resolvedType} + } } else { logger.Warn().Msgf("invalid configuration: type %s not supported (should have been spotted when validating configuration)", fieldType) } @@ -95,7 +101,12 @@ func (s *schemaRegistry) populateSchemaFromStaticConfiguration(indexConfiguratio continue } if resolvedType, valid := ParseType(field.Type.AsString()); valid { - fields[FieldName(field.Name.AsString())] = Field{PropertyName: FieldName(field.Name.AsString()), InternalPropertyName: FieldName(field.Name.AsString()), Type: resolvedType} + // TODO replace with notion of ephemeral types + if resolvedType.Equal(TypePoint) { + fields[FieldName(field.Name.AsString())] = Field{PropertyName: FieldName(field.Name.AsString()), InternalPropertyName: FieldName(strings.Replace(field.Name.AsString(), ".", "::", -1)), Type: resolvedType} + } else { + fields[FieldName(field.Name.AsString())] = Field{PropertyName: FieldName(field.Name.AsString()), InternalPropertyName: FieldName(field.Name.AsString()), Type: resolvedType} + } } else { logger.Warn().Msgf("invalid configuration: type %s not supported (should have been spotted when validating configuration)", field.Type.AsString()) } @@ -130,13 +141,16 @@ func (s *schemaRegistry) populateSchemaFromTableDefinition(definitions map[strin logger.Debug().Msgf("loading schema for table %s", indexName) for _, column := range tableDefinition.Columns { - if _, exists := fields[FieldName(column.Name)]; !exists { - if quesmaType, found2 := s.dataSourceTypeAdapter.Convert(column.Type); found2 { - fields[FieldName(column.Name)] = Field{PropertyName: FieldName(column.Name), InternalPropertyName: FieldName(column.Name), Type: quesmaType} + propertyName := FieldName(strings.Replace(column.Name, "::", ".", -1)) + if existing, exists := fields[propertyName]; !exists { + if quesmaType, resolved := s.dataSourceTypeAdapter.Convert(column.Type); resolved { + fields[propertyName] = Field{PropertyName: propertyName, InternalPropertyName: FieldName(column.Name), Type: quesmaType} } else { logger.Debug().Msgf("type %s not supported, falling back to text", column.Type) - fields[FieldName(column.Name)] = Field{PropertyName: FieldName(column.Name), InternalPropertyName: FieldName(column.Name), Type: TypeText} + fields[propertyName] = Field{PropertyName: propertyName, InternalPropertyName: FieldName(column.Name), Type: TypeText} } + } else { + fields[propertyName] = Field{PropertyName: propertyName, InternalPropertyName: FieldName(column.Name), Type: existing.Type} } } }