FromAsString & ToAsString properties were not serializing

[dashkan]

The underlying values for from_as_string & to_as_string properties in JSON.NET are DateTime objects therefor the "reader.value as string" would always be null. Change the code to ALWAYS deserialize as string.

Also updated unit tests to validate both FromAsString and ToAsString properties.

[Mpdreamz]

Awesome @ashkandaie ty for taking the time to send in a fix for this!

Can you sign our CLA found here: http://www.elasticsearch.org/contributor-agreement/

That way we are allowed to pull your bits in 👍

[dashkan]

Absolutely. Thank you for the great product.

Signed.

Sent from my phone.
Please excuse any typos.
Ashkan Daie

On Oct 15, 2014, at 1:04 PM, Martijn Laarman <[email protected]mailto:[email protected]> wrote:

Awesome @ashkandaiehttps://github.com/ashkandaie ty for taking the time to send in a fix for this!

Can you sign our CLA found here: http://www.elasticsearch.org/contributor-agreement/

That way we are allowed to pull your bits in [:+1:]

Reply to this email directly or view it on GitHubhttps://github.com//pull/994#issuecomment-59267017.

[dashkan]

Martijn,

I think this might be a bigger issue than what I originally thought.

Anytime JSON.Net reads a property and can interpret as a DateTime it does so.

Just as a quick test, I set the key to a date/time value:

var results = this.Client.Search(s => s
.Size(0)
.Aggregations(a=>a
.DateRange("my_geod", dh=>dh
.Field(p=>p.StartedOn)
.Ranges(
r => r.To("now-10M/M").Key("2013-12-01T00:00:00.000Z"),
r=>r.From("now-10M/M")
)
)
)
);

The returned result which should be the string literal of "2013-12-01T00:00:00.000Z” has been replaced by “12/1/2013 12:00:00 AM” because the value was read in as a date and then a ToString was called on the date:

          private IAggregation GetKeyedBucketItem(JsonReader reader, JsonSerializer serializer)
          {
                 reader.Read();
                 var key = reader.Value;
                 reader.Read();
                 var property = reader.Value as string;
                 if (property == "from" || property == "to")
                       return GetRangeAggregation(reader, serializer, key.ToString());

I can definitely take a look & see if I find more instances of this.

Regards,
Ashkan

From: Martijn Laarman [mailto:[email protected]]
Sent: Wednesday, October 15, 2014 1:05 PM
To: elasticsearch/elasticsearch-net
Cc: Ashkan Daie
Subject: Re: [elasticsearch-net] FromAsString & ToAsString properties were not serializing (#994)

Awesome @ashkandaiehttps://github.com/ashkandaie ty for taking the time to send in a fix for this!

Can you sign our CLA found here: http://www.elasticsearch.org/contributor-agreement/

That way we are allowed to pull your bits in [:+1:]

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/994#issuecomment-59267017.

[Mpdreamz]

H @ashkandaie I see you attached two more commits but one of them is a revert?

Mind explaining where you got stuck, what roadblock you hit?

[dashkan]

Hi Martijn,

Sorry for the delayed response. I tried to fix other areas where json.net would be serializing strings as dates. I committed w/o running the unit tests and later noticed that one of the unit tests was failing.

I have been bogged down w/ the paying job for the last week or so and have not had a chance to look at it.

I’ll take a stab again in a couple of days.

The reverted commit should address the bug w/ keys in date ranges.

Ashkan

From: Martijn Laarman [mailto:[email protected]]
Sent: Wednesday, October 22, 2014 12:53 AM
To: elasticsearch/elasticsearch-net
Cc: Ashkan Daie
Subject: Re: [elasticsearch-net] FromAsString & ToAsString properties were not serializing (#994)

H @ashkandaiehttps://github.com/ashkandaie I see you attached two more commits but one of them is a revert?

Mind explaining where you got stuck, what roadblock you hit?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/994#issuecomment-60048436.

[dashkan]

Martijn,

Is there a reason the PR has not been merged? The fix does address the issue for aggregates.

I would like to move to the nuget version of ES ASAP.

Best,
Ashkan

[Mpdreamz]

Hi @ashkandaie sorry for the delay I've reimplemented your changes manually as per:

#1191

Ty for reporting and sending a PR for this issue 👍 closing this PR in favor of #1191