-
Notifications
You must be signed in to change notification settings - Fork 25
DOCSP-41170 - Clarify string ID generation #365
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
DOCSP-41170 - Clarify string ID generation #365
Conversation
✅ Deploy Preview for mongodb-docs-csharp ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM w/ two small things
|
||
.. list-table:: | ||
:header-rows: 1 | ||
:widths: 10 10 | ||
:widths: 25 75 | ||
|
||
* - ``Id`` Field Data Type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't immediately see why, but this is rendering without a space after "Id". Maybe because of the monospace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i couldn't get this to look right so i just removed the monospace. seems like that actually matches the style guide too
|
||
* - ``ObjectId`` | ||
- The driver automatically uses the ``ObjectIdGenerator`` type for ``Id`` fields with | ||
the ``ObjectId`` data type, like the one in the following code example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://www.mongodb.com/docs/meta/style-guide/terminology/alphabetical-terms/#std-term-like
the ``ObjectId`` data type, like the one in the following code example: | |
the ``ObjectId`` data type, such as the one in the following code example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
boo this busted rule
Could/Should we mention the example mentioned in the ticket as well that would allow users to define the generator in the attribute itself |
For my understanding: What is the use case for this attribute? Do you need to do this in addition to the |
I'm actually not sure. Maybe @rstam can help. What's the difference between using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
They are not the same. Using BsonId with StringObjectIdGenerator without configuring the representation would store the ObjectIds in the database as strings. BsonRepresentation(BsonType.ObjectId) stores the strings as ObjectIds in the database. They are used together. Though unless you have changed the default conventions there is a convention that automatically sets the id generator for a string stored as an ObjectId to the StringObjectIdGenerator . |
Thanks Robert. @mongoKart I think its worth adding in a callout then in case a user wants to store the ID as a string along with representing it as a string in their POCO. |
Co-authored-by: Jordan Smith <45415425+jordan-smith721@users.noreply.github.com>
@rishitb-mongodb @rstam Could you take another look? |
default value. Instead, the driver generates a unique ID value for the property. | ||
|
||
When you apply the ``[BsonId]`` attribute to a property, you can pass the ``IdGenerator`` | ||
argument to specify the type of ID value the driver generates for the corresponding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not "the type of ID value"
The type is already determined by the type of the property.
What are you specifying is which generator to use to generate an Id value when needed.
} | ||
|
||
* - ``string`` | ||
- To serialize the ``Id`` property as a ``string``, apply the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applying the [BsonId]
attribute is NOT what causes this property to be serialized as a string.
All strings are serialized as strings UNLESS the [BsonElement]
attribute specifies a different representation.
The ONLY thing this [BsonId]
attribute is specifying is which id generator to use (beside identifying this as the ID field of course).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking at the code for the Generator classes, I see that I've been conflating the ID generation and serialization steps.
To serialize the ``Id`` property as an ``ObjectId``, apply the | ||
``[BsonRepresentation(BsonType.ObjectId)]`` attribute to the property, as shown | ||
in the following code example. In this case, if the property contains the value | ||
``null``, the driver uses the ``ObjectIdGenerator`` type to generate a unique |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it uses the StringObjecIdGenerator
.
The ObjectIdGenerator
can only be used with properties of type ObjectId
.
|
||
If you apply both the ``[BsonId(IdGenerator = typeof(StringObjectIdGenerator))]`` | ||
and ``[BsonRepresentation(BsonType.ObjectId)]`` attributes to the ``Id`` property, | ||
the driver generates an ``ObjectId`` value for the field. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this is worded the best way.
If you apply both you have simply done TWO different things:
- Configured the representation of the ID in the database
- Configured the id generator to use for missing ID values
@@ -247,7 +248,7 @@ The following code sample maps the ``Identifier`` property to the | |||
public string Identifier { get; set; } | |||
} | |||
|
|||
.. warning:: Multiple Id Fields | |||
.. warning:: Multiple ID Fields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed most occurrences of 'Id' to 'ID' since you can use any name for an ID property
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But... the C#/.Net naming convention is to use Id
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After talking to @jordan-smith721, keeping as 'ID' to comply with our Style Guide
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have two minor comments but if you want to push as is LGTM.
@@ -247,7 +248,7 @@ The following code sample maps the ``Identifier`` property to the | |||
public string Identifier { get; set; } | |||
} | |||
|
|||
.. warning:: Multiple Id Fields | |||
.. warning:: Multiple ID Fields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But... the C#/.Net naming convention is to use Id
.
Every document in a MongoDB collection must have a unique ID. When you serialize an object | ||
to a collection, if its ID property contains the default | ||
value for its data type (usually ``null``), the {+driver-short+} doesn't serialize the | ||
default value. Instead, the driver generates a unique ID value and assigns it to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The C# driver TRIES to generate a unique Id value but can onlly do so in cases where it knows what Id generator to use.
Co-authored-by: Jordan Smith <45415425+jordan-smith721@users.noreply.github.com> (cherry picked from commit 30ff13b)
Co-authored-by: Jordan Smith <45415425+jordan-smith721@users.noreply.github.com> (cherry picked from commit 30ff13b)
Co-authored-by: Jordan Smith <45415425+jordan-smith721@users.noreply.github.com> (cherry picked from commit 30ff13b)
Co-authored-by: Jordan Smith <45415425+jordan-smith721@users.noreply.github.com> (cherry picked from commit 30ff13b)
Co-authored-by: Jordan Smith <45415425+jordan-smith721@users.noreply.github.com> (cherry picked from commit 30ff13b)
Co-authored-by: Jordan Smith <45415425+jordan-smith721@users.noreply.github.com> (cherry picked from commit 30ff13b)
Co-authored-by: Jordan Smith <45415425+jordan-smith721@users.noreply.github.com> (cherry picked from commit 30ff13b)
Co-authored-by: Jordan Smith <45415425+jordan-smith721@users.noreply.github.com> (cherry picked from commit 30ff13b)
Co-authored-by: Jordan Smith <45415425+jordan-smith721@users.noreply.github.com> (cherry picked from commit 30ff13b) Co-authored-by: Mike Woofter <108414937+mongoKart@users.noreply.github.com>
Co-authored-by: Jordan Smith <45415425+jordan-smith721@users.noreply.github.com>
Pull Request Info
PR Reviewing Guidelines
JIRA - https://jira.mongodb.org/browse/DOCSP-41170
Staging Links
Self-Review Checklist