mongodb/specifications

upsertedId & uspertedCount in UpdateResult could be confusing

richardscarrott opened this issue · 2 comments

I've always read "upsert" to mean "update or insert" (which could be wrong of course) however UpdateResult implies it means "insert" given that UpdateResult.upsertedCount and UpdateResult.upsertedId are only set if an insert occurred.

I wonder if it'd be clearer if it was UpdateResult.insertedCount and UpdateResult.insertedId which would align more with
$setOnInsert (i.e. it's not $setOnUpsert)?

Perhaps a minor grievance but I always find myself having to double check + test how upsertedCount behaves.

https://github.com/mongodb/specifications/blob/021cbc80e1e444023fd05d8092df4546e639db40/source/crud/crud.rst

/**
   * The number of documents that were upserted.
   *
   * NOT REQUIRED: Drivers may choose to not provide this property so long as
   * it is always possible to infer whether an upsert has taken place. Since
   * the "_id" of an upserted document could be null, a null "upsertedId" may
   * be ambiguous in some drivers. If so, this field can be used to indicate
   * whether an upsert has taken place.
   */
  upsertedCount: Int64;

  /**
   * The identifier of the inserted document if an upsert took place.
   */
  upsertedId: any;

"Upsert" is indeed a portmanteau of "update" and "insert" and predates MongoDB. AFAIK, driver documentation is consistent about always referring to an insert occurring through an update command (i.e. with upsert: true) as an "upsert".

I don't have an explanation for how the name for $setOnInsert was chosen by the query team, but I can think of at least two justifications for the current field names in drivers:

  • The server response for update commands includes an upserted array, which is used to derive the driver result fields.
  • upsertedCount is also used in BulkWriteResult, which already has an insertedCount field that is specific to documents inserted via insert commands. This also extends to the upsertedIds and insertedIds maps in BulkWriteResult. Inconsistent naming between UpdateResult and BulkWriteResult would likely invite more confusion than the situation you're describing.

I don't have an explanation for how the name for $setOnInsert was chosen by the query team ...

$setOnInsert was introduced via SERVER-340 and seems to have considered a number of possible alternate names ($setDef, $onInsert, $setIfAbsent). As a new document is being inserted (regardless of whether it is via an insert or update+upsert) it is a clearer directive than "setOnUpsert".

Closing this issue out as it doesn't appear any changes are being suggested, however this issue can be reopened later if needed.