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-255 Enable disabling the id to _id field rename in embedded documents #3332

Merged
merged 4 commits into from
Apr 9, 2025

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Apr 1, 2025

Fix PHPORM-255
Fix #3184

To disable the automatic conversion of id to _id and store id fields in the database for embedded documents, set the connection setting using the new method:

DB::connection('mongodb')->setRenameEmbeddedIdField(false);

Checklist

  • Add tests and ensure they pass

Copy link

@stakx stakx left a comment

Choose a reason for hiding this comment

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

Thanks for doing this PR, really happy to see any progress regarding this issue!

Copy link

@mshamaseen mshamaseen left a comment

Choose a reason for hiding this comment

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

It would be great if we could add an option in the config file to disable this behaviour.

@GromNaN GromNaN marked this pull request as ready for review April 7, 2025 18:32
@GromNaN GromNaN requested a review from a team as a code owner April 7, 2025 18:32
@GromNaN GromNaN requested a review from jmikola April 7, 2025 18:32
Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

I don't think my questions warrant any changes, but feel free to ping me for a second look if needed.

@@ -1805,7 +1807,7 @@ private function aliasIdForQuery(array $values): array

foreach ($values as &$value) {
if (is_array($value)) {
$value = $this->aliasIdForQuery($value);
$value = $this->aliasIdForQuery($value, false);
Copy link
Member

Choose a reason for hiding this comment

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

Note that this appears to be the only recursive call to aliasIdForQuery(). All other calls (e.g. preparing projections, wheres) are invoked at the root level.

@@ -1823,10 +1825,13 @@ private function aliasIdForQuery(array $values): array
*
* @template T of array|object
*/
public function aliasIdForResult(array|object $values): array|object
public function aliasIdForResult(array|object $values, bool $root = true): array|object
Copy link
Member

Choose a reason for hiding this comment

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

There appear to be two calls to aliasIdForResult() from MongoDB\Laravel\Eloquent\Builder::raw() (see: search results).

The context suggests that you're only processing a single document, but in one case you call iterator_to_array() on a cursor (which suggests it'd be handling multiple results instead of just one).

Neither of the aliasIdForResult() calls there seem affected by this change, since $root: false is only used when recursing, but it'd be worth confirming.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, that's fixed by using array_map.

Copy link
Member

Choose a reason for hiding this comment

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

Just to confirm, this fix was necessary to get the test case for multiple results passing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Previously, the function was called with $root = true on the list of document, and then each document root had $root = false because the function was called recursively. Using array_map, I'm calling aliasIdForResult on the documents directly, instead of the collection of documents.

@@ -1600,12 +1599,38 @@ public static function getEloquentMethodsNotSupported()
yield 'orWhereIntegerNotInRaw' => [fn (Builder $builder) => $builder->orWhereIntegerNotInRaw('id', ['1a', 2])];
}

private static function getBuilder(): Builder
public function testRenameEmbeddedIdFieldCanBeDisabled()
Copy link
Member

Choose a reason for hiding this comment

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

Does this test exercise both aliasIdForQuery and aliasIdForResult()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for this question, I added a full test to cover both.

Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

One question so I can understand what the array_map() addition actually fixed, but this LGTM.

@@ -1823,10 +1825,13 @@ private function aliasIdForQuery(array $values): array
*
* @template T of array|object
*/
public function aliasIdForResult(array|object $values): array|object
public function aliasIdForResult(array|object $values, bool $root = true): array|object
Copy link
Member

Choose a reason for hiding this comment

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

Just to confirm, this fix was necessary to get the test case for multiple results passing?

@GromNaN GromNaN merged commit bd8705a into mongodb:5.x Apr 9, 2025
70 checks passed
@GromNaN GromNaN deleted the PHPORM-255 branch April 9, 2025 14:12
@GromNaN GromNaN added this to the 5.3 milestone Apr 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Options to ignore auto conversion id to _id
5 participants