Skip to content
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

PHPORM-147 Make id an alias for _id #3040

Merged
merged 3 commits into from
Aug 20, 2024
Merged

PHPORM-147 Make id an alias for _id #3040

merged 3 commits into from
Aug 20, 2024

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Jul 10, 2024

Fix PHPORM-147
Fix #3022 (PHPORM-201)
Fix #2489

Laravel and Eloquent uses id as conventional field name for models and various queries. MongoDB's primary key is called _id. The name is source of various issues with Laravel packages:

Breaking change: it is no longer possible to set a different value for id and _id. If you need a distinct ID field, use an other name.

Checklist

  • Add tests and ensure they pass
  • Add an entry to the CHANGELOG.md file
  • Update documentation for new features

@GromNaN GromNaN force-pushed the PHPORM-147 branch 2 times, most recently from e7015c0 to 7535a88 Compare July 10, 2024 13:00
@GromNaN GromNaN marked this pull request as ready for review July 10, 2024 13:01
@GromNaN GromNaN requested a review from a team as a code owner July 10, 2024 13:01
@GromNaN GromNaN requested a review from alcaeus July 10, 2024 13:01
@bisht2050
Copy link
Contributor

Should this target a major release?

@masterbater
Copy link
Contributor

Seems like a minor breaking change, since id is already correlated with _id. Hopefully this get merged soon. This is a must update that can also unblock more laravel packages together with DocumentModel trait.
@GromNaN 🫡🙏🏻thanks for being thoughtful of the community

Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

I'll admit, I'm not entirely convinced we want to break BC in a minor release, but given the state of the whole _id vs. id situation, it might be better to just get this over with.

Correct me if I'm wrong, but as I understand the current situation is:

  • Models always have an _id field
  • The value of _id is returned when fetching the id field
  • Using id in the query builder is already changed to use _id

With this change, we're using the value of id for _id when saving models, but throw if there are different values for id and _id. This ensures that other libraries are able to query MongoDB models the same way they query relational models. Did I understand all of this correctly?

CHANGELOG.md Outdated Show resolved Hide resolved
@GromNaN
Copy link
Member Author

GromNaN commented Jul 11, 2024

Using id in the query builder is already changed to use _id

_id was already used by the Query\Builder::find method. But a where('id', <value>) was not converted to { "_id": <value> }. With Eloquent models, there is already an alias when the attribute id is read

public function getIdAttribute($value = null)
{
// If we don't have a value for 'id', we will use the MongoDB '_id' value.
// This allows us to work with models in a more sql-like way.
if (! $value && array_key_exists('_id', $this->attributes)) {
$value = $this->attributes['_id'];
}

@GromNaN GromNaN changed the base branch from 4.8 to 5.0 July 22, 2024 12:32
@GromNaN GromNaN requested a review from a team as a code owner August 19, 2024 16:44
@GromNaN GromNaN requested a review from norareidy August 19, 2024 16:44
@github-actions github-actions bot added the docs label Aug 19, 2024
@GromNaN GromNaN added this to the 5.0 milestone Aug 19, 2024
@GromNaN GromNaN force-pushed the PHPORM-147 branch 4 times, most recently from dbee100 to 6efb2a4 Compare August 20, 2024 10:06
Copy link
Contributor

@rustagir rustagir left a comment

Choose a reason for hiding this comment

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

lgtm

@GromNaN GromNaN merged commit a453f8a into mongodb:5.0 Aug 20, 2024
26 checks passed
@GromNaN GromNaN deleted the PHPORM-147 branch August 20, 2024 16:13
@GromNaN
Copy link
Member Author

GromNaN commented Sep 4, 2024

I was on the fence about unsetting _id or not. That's why I left this comment:

//unset($values['_id']);

It makes it impossible to work with _id in result as it would be natively, while _id or id can both be used in a query.

Thanks for your feedback. It seems that we should unset _id indead.

@masterbater
Copy link
Contributor

masterbater commented Sep 4, 2024

I dont know if this an issue or anything that can be improved for now I made adjustment on my side by unsetting the _id field
Example

Address model
User with address field

$address = Address::find(objectIdhere);
  // unset($address ['_id']); Solution
$users = User::insert([
                        'name' => $user->fullname,
                        'username' => $user->username,
                        'address' => $address->toArray()])

Cannot have both "id" and "_id" fields. {"exception":"[object] (InvalidArgumentException(code: 0): Cannot have both "id" and "_id" fields. at /app/vendor/mongodb/laravel-mongodb/src/Query/Builder.php:1620)
It seems good that it can detect also nested documents that has both id and _id, but is there a way it only returns id or do we just need to keep unsettting the _id field

@GromNaN
Copy link
Member Author

GromNaN commented Sep 4, 2024

We can also detect when _id === id in a query and unset(id). That would solve your issue.

@masterbater
Copy link
Contributor

I was on the fence about unsetting _id or not. That's why I left this comment:

//unset($values['_id']);

It makes it impossible to work with _id in result as it would be natively, while _id or id can both be used in a query.

Thanks for your feedback. It seems that we should unset _id indeed.

Thanks for replying so fast, I edited my question now its below to the answer hahaha.

So the aliasIdForResult will just return the id field, but we cant access the native _id Objectid. This would be in line to Laravel behavior but probably needs to be documented. This is acceptable though just like prisma uses id for mongodb too.

@masterbater
Copy link
Contributor

masterbater commented Sep 4, 2024

We can also detect when _id === id in a query and unset(id). That would solve your issue.

When using v5 I always assume that _id is non existent so I always use id both for saving data and retrieving it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants