Skip to content
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

types.Property is not correctly unmarshaled when child mapping elements have no "type" field. #696

Closed
ngicks opened this issue Jul 11, 2023 · 7 comments

Comments

@ngicks
Copy link

ngicks commented Jul 11, 2023

Types which contain types.Property are not correctly unmarshaled when input mapping denifitions have no "type" field.

For example this part, types.TypeMapping is not handling absence of "type" field.

switch kind["type"] {
case "binary":
oo := NewBinaryProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "boolean":
oo := NewBooleanProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "{dynamic_property}":
oo := NewDynamicProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "join":
oo := NewJoinProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "keyword":
oo := NewKeywordProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "match_only_text":
oo := NewMatchOnlyTextProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "percolator":
oo := NewPercolatorProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "rank_feature":
oo := NewRankFeatureProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "rank_features":
oo := NewRankFeaturesProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "search_as_you_type":
oo := NewSearchAsYouTypeProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "text":
oo := NewTextProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "version":
oo := NewVersionProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "wildcard":
oo := NewWildcardProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "date_nanos":
oo := NewDateNanosProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "date":
oo := NewDateProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "aggregate_metric_double":
oo := NewAggregateMetricDoubleProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "dense_vector":
oo := NewDenseVectorProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "flattened":
oo := NewFlattenedProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "nested":
oo := NewNestedProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "object":
oo := NewObjectProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "completion":
oo := NewCompletionProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "constant_keyword":
oo := NewConstantKeywordProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "alias":
oo := NewFieldAliasProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "histogram":
oo := NewHistogramProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "ip":
oo := NewIpProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "murmur3":
oo := NewMurmur3HashProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "token_count":
oo := NewTokenCountProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "geo_point":
oo := NewGeoPointProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "geo_shape":
oo := NewGeoShapeProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "point":
oo := NewPointProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "shape":
oo := NewShapeProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "byte":
oo := NewByteNumberProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "double":
oo := NewDoubleNumberProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "float":
oo := NewFloatNumberProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "half_float":
oo := NewHalfFloatNumberProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "integer":
oo := NewIntegerNumberProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "long":
oo := NewLongNumberProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "scaled_float":
oo := NewScaledFloatNumberProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "short":
oo := NewShortNumberProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "unsigned_long":
oo := NewUnsignedLongNumberProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "date_range":
oo := NewDateRangeProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "double_range":
oo := NewDoubleRangeProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "float_range":
oo := NewFloatRangeProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "integer_range":
oo := NewIntegerRangeProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "ip_range":
oo := NewIpRangeProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
case "long_range":
oo := NewLongRangeProperty()
if err := localDec.Decode(&oo); err != nil {
return err
}
s.Properties[key] = oo
default:
if err := localDec.Decode(&s.Properties); err != nil {
return err
}
}

https://www.elastic.co/guide/en/elasticsearch/reference/current/object.html

You are not required to set the field type to object explicitly, as this is the default value.

As stated in the official document, this is not correct behavior.

@ngicks ngicks changed the title types.Property is not correctly unmarshaled when child has no "type" field. types.Property is not correctly unmarshaled when child mapping elements have no "type" field. Jul 11, 2023
@ngicks
Copy link
Author

ngicks commented Jul 12, 2023

Wher do I need to look into? There should be code generator which has generated these code as it states so. But I've not yet found it.

@jsoriano
Copy link
Member

I am also finding issues using the simulate template API, it seems to incorrectly unmarshal nested properties.

I think the problem is in

default:
if err := localDec.Decode(&s.Properties); err != nil {
return err
}

Replacing this part with the following code seems to fix the issue:

                                default:
                                        oo := NewObjectProperty()
                                        if err := localDec.Decode(&oo); err != nil {
                                                return err
                                        }
                                        s.Properties[key] = oo

@Anaethelion do you know where we can change the generator to fix this? Thanks!

Anaethelion added a commit that referenced this issue Jul 26, 2023
@ngicks
Copy link
Author

ngicks commented Jul 26, 2023

I thought a correct fix would be

				ty, ok  := kind["type"] 
                                if !ok {
                                	ty = "object"
                                }

				switch ty {
				case "binary":
					oo := NewBinaryProperty()
					if err := localDec.Decode(&oo); err != nil {
						return err
					}
					s.Properties[key] = oo
				case "boolean":
					oo := NewBooleanProperty()
					if err := localDec.Decode(&oo); err != nil {
						return err
					}
					s.Properties[key] = oo
// ... rest of cases ...
				default:
					return fmt.Errorf("unknown type %s",  ty)
				}

@Anaethelion
Copy link
Contributor

Since the specification has the @non_exhaustive tag, you both are right.

I've already commited a fallback in the PR, I'm working on a follow-up to handle both the default case and unknown members.

@ngicks
Copy link
Author

ngicks commented Jul 26, 2023

I'm glad to know that I didn't know that part.

Would it be like this?

				ty, ok  := kind["type"] 
                                if !ok {
                                	ty = "object"
                                }

				switch ty {
				case "binary":
					oo := NewBinaryProperty()
					if err := localDec.Decode(&oo); err != nil {
						return err
					}
					s.Properties[key] = oo
				case "boolean":
					oo := NewBooleanProperty()
					if err := localDec.Decode(&oo); err != nil {
						return err
					}
					s.Properties[key] = oo
// ... rest of cases ...
				default:
					// plugins might be able to handle this.
					var oo any
					if err := localDec.Decode(&oo); err != nil {
						return err
					}
					s.Properties[key] = oo
				}

Thank you for the fix. I'll wait for next release.

@jsoriano
Copy link
Member

Thanks @Anaethelion!

Anaethelion added a commit that referenced this issue Jul 26, 2023
* TypedClient: remove ndjson helper, not yet ready

* TypedClient: fix #696

* TypedClient: add handling for default type in unions (Property & Analyzer)
@Anaethelion
Copy link
Contributor

Released as part of 8.9.0!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants