Skip to content

fix(database): prevent deeply nested has many map adding extra empty object#1995

Merged
brendt merged 4 commits intotempestphp:3.xfrom
nunntom:deeply-nested-has-many-map-bug
Mar 19, 2026
Merged

fix(database): prevent deeply nested has many map adding extra empty object#1995
brendt merged 4 commits intotempestphp:3.xfrom
nunntom:deeply-nested-has-many-map-bug

Conversation

@nunntom
Copy link
Copy Markdown
Contributor

@nunntom nunntom commented Feb 19, 2026

closes #1994

  • Added failing assertion to existing test showing the bug
  • Removed line in SelectModelMapper that adds initial empty model. It doesn't seem necessary since the later $data->set adds the full nested path anyway. Please correct me if I'm wrong or if I broke anything. Tests are all still passing (including the failing one I added).

Comment thread packages/database/src/Mappers/SelectModelMapper.php
Comment thread packages/database/src/Mappers/SelectModelMapper.php
@brendt brendt marked this pull request as draft March 9, 2026 08:22
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 9, 2026

Benchmark Results

Comparison of deeply-nested-has-many-map-bug against 3.x (5563eac1332581f39b908f0e4b23adf53167c282).

Open to see the benchmark results
Benchmark Set Mem. Peak Time Variability
ViewRenderBench(benchPlainHtml) - 21.481mb 0.00% 488.857μs -5.87% ±3.67% +27.95%

Generated by phpbench against commit 7c8dee2

@brendt
Copy link
Copy Markdown
Member

brendt commented Mar 19, 2026

Ping me when ready for review :)

@nunntom
Copy link
Copy Markdown
Contributor Author

nunntom commented Mar 19, 2026

Yes please, ready for review @brendt

@brendt brendt marked this pull request as ready for review March 19, 2026 10:08
@brendt brendt merged commit 469e50a into tempestphp:3.x Mar 19, 2026
75 checks passed
@brendt
Copy link
Copy Markdown
Member

brendt commented Mar 19, 2026

I wonder if we might have missed a test for this piece of code originally, because I'm kind of skeptical about removing it and everything just… continuing to work. Or, it could also be a leftover from an old iteration. I honestly don't remember.

So I'm going to merge this, and we'll see if something goes wrong :)

@nunntom
Copy link
Copy Markdown
Contributor Author

nunntom commented Mar 19, 2026

Yeah, I don't get what that piece of code could have originally been for. My reasoning is doesn't the later $data->set recursively add any needed unset parents when adding a new child? Which would make it unnecessary to preemptively add the empty parent.

If it's any comfort I've been running a composer-patch of this change for a few weeks and haven't seen any issues, though I'm probably not covering anywhere near all use-cases. Fingers crossed!

Thanks for merging! :)

@brendt
Copy link
Copy Markdown
Member

brendt commented Mar 19, 2026

I had Claude take a look at it for 10 minutes because I genuinely didn't know anymore.

I now think it was added to deal with setting an empty array for the first iteration so that the array itself was present. However, we're now using $data->set($key, $value); which also does these "initialize missing nested array checks" for us, so the buggy part wasn't needed anymore.

At least, that's my understanding for now. We'll see how it goes :)

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.

Deeply nested has-many map creates additional empty relation object

2 participants