BulkUpdateDescriptor & DocAsUpsert Not Working in Latest Dev Branch

[nariman-haghighi]

It seems .Object was removed on BulkUpdateDescriptor recently and the following usage:

descriptor.Update<MediaStreamEntry>(u => u.Index("StreamEntry").Document(m).DocAsUpsert(true));

... is now generating: script or doc is missing. We previously had .Document(m).Object(m).DocAsUpsert(true) working correctly.

[gmarz]

Hey @nariman-haghighi, you bring up an interesting issue here. Yes, Object() has been removed and replaced with Document(), but they also function differently. Document() is only used to infer the ID of the document that you want to apply a partial update to.

There is now an UpdatePartial() method that acts as the old Object() (i.e. serializes to "doc"). It's a bit confusing, and I'm not sure the exact reasoning behind it- will have to ping @Mpdreamz and get his take on it. My guess is that in most uses cases, if you are applying a partial update, you don't want to pass the entire C# object, as some of the fields that aren't populated will override your document in ES with the default values. So you'd want to do this instead:

descriptor.Update<MediaStreamEntry, object>(op => op
    .Document(new MediaStreamEntry { Id = id })
    .PartialUpdate(new { propertyToUpdate = "value" })
    .DocAsUpsert(true)

However, in your case, I think this might work:

descriptor.Update<MediaStreamEntry>(op => op
    .Document(m)
    .PartialUpdate(m)
    .DocAsUpsert(true)

Or even:

descriptor.Update<MediaStreamEntry>(op => op
    .Id(m.Id) // Or whatever property holds your document id
    .PartialUpdate(m)
    .DocAsUpsert(true)

Let me know how that goes and thank you for bringing this to our attention. We'll definitely have rethink this for the GA release, or mention it in the breaking changes section of the release notes, as it's more than just renaming Object to Document.

[nariman-haghighi]

Yes, the 2nd example above Document(m).PartialUpdate(m) works as expected without sending the full document twice. Because we're habituated to the behaviour pre-partial updates, our documents are designed with nesting so that each update provides only the relevant sub-document (and null for everything) else so as to not override the fields in ES that are not a part of the update (NULL should not replace existing values). Basically we can pass m in whole and not worry about it.

[gmarz]

Ah, gotcha. Great, glad that worked.

[Mpdreamz]

Dang, sorry for the switcheroo going on here @nariman-haghighi

It was not on my radar that .Document() used to fill the role of .PartialUpdate(), we'll be sure to update the breaking changes for 1.0 to reflect this.

Moving forward I very much prefer the new syntax over the old though e.g:

descriptor.Update<MediaStreamEntry>(op => op
    .Document(m)
    .PartialUpdate(m)
    .DocAsUpsert(true)

over

descriptor.Update<MediaStreamEntry>(op => op
    .Object(m)
    .Document(m)
    .DocAsUpsert(true)

Thanks for catching this.

Its also very interesting to hear that you've designed your POCO's to specifically allow for this, I never envisioned this to be honest. The update that only takes one type was mainly there for the cases where you want to infer from <T> and update using scripts.

[nariman-haghighi]

And what about UpdateDescriptor? It's still using the .Object(m).Document(m).DocAsUpsert(true) format.

[gmarz]

@nariman-haghighi Yup, they are inconsistent. We're going to have to go back to the drawing border here and think of a better solution for the update api. Leaving this issue open until then. Thanks again.

[Mpdreamz]

@nariman-haghighi @gmarz I opened a PR to try and streamline our API around both updates.

#795

Would love your comments.

[Mpdreamz]

Closing this now that #795 has been pulled in.