Skip to content

Fix for covariant search date parsing #2643

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

Merged
merged 3 commits into from
Mar 2, 2017
Merged

Conversation

gmarz
Copy link
Contributor

@gmarz gmarz commented Mar 1, 2017

This commit fixes an issue where indexing dates as strings and pulling
them back out using covariant search would result in the format being
different due to the default DateParseHandling serialization settings.

ConcereteTypeConverter now temporarily sets DateParseHandling to None
when deserializing covariant results.

/cc @gmoskovicz

This commit fixes an issue where indexing dates as strings and pulling
them back out using covariant search would result in the format being
different due to the default DateParseHandling serialization settings.

ConcereteTypeConverter now temporarily sets DateParseHandling to None
when deserializing covariant results.
Copy link
Member

@Mpdreamz Mpdreamz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nag but other than that LGTM

@@ -94,7 +94,10 @@ internal static class ConcreteTypeConverter

private static JObject CreateIntermediateJObject(JsonReader reader)
{
var original = reader.DateParseHandling;
reader.DateParseHandling = DateParseHandling.None;
Copy link
Member

@Mpdreamz Mpdreamz Mar 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment why we feel this is not side effecty.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great fix. ++ on adding a comment.

Quick question, where do we use the StatefulDeserialization class? Only for Covariant Search type? Just trying to understand whether this can affect other functionality.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CovariantSearch needs ConcreteTypeConverter, in order to create an isolated serializer that has access to state we call the Stateful deserialization factory on ISerializerFactory and we append a new instance of ConcreteTypeConverter. Search/MultiGet/MultiSearch/SearchTemplate all possibly call this (but only if needed)

@gmarz gmarz merged commit a4890fc into 5.x Mar 2, 2017
@gmarz gmarz deleted the fix/covariant-search-date-parsing branch March 2, 2017 15:44
gmarz added a commit that referenced this pull request Mar 2, 2017
* Fix for covariant search date parsing

This commit fixes an issue where indexing dates as strings and pulling
them back out using covariant search would result in the format being
different due to the default DateParseHandling serialization settings.

ConcereteTypeConverter now temporarily sets DateParseHandling to None
when deserializing covariant results.

* add comment explaining the fix
gmarz added a commit that referenced this pull request Mar 2, 2017
* Fix for covariant search date parsing

This commit fixes an issue where indexing dates as strings and pulling
them back out using covariant search would result in the format being
different due to the default DateParseHandling serialization settings.

ConcereteTypeConverter now temporarily sets DateParseHandling to None
when deserializing covariant results.

* add comment explaining the fix
@gmarz
Copy link
Contributor Author

gmarz commented Mar 2, 2017

Merged to 2.x, 5.x, and master

awelburn pushed a commit to Artesian/elasticsearch-net that referenced this pull request Nov 6, 2017
* Fix for covariant search date parsing

This commit fixes an issue where indexing dates as strings and pulling
them back out using covariant search would result in the format being
different due to the default DateParseHandling serialization settings.

ConcereteTypeConverter now temporarily sets DateParseHandling to None
when deserializing covariant results.

* add comment explaining the fix
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

Successfully merging this pull request may close these issues.

3 participants