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

Null value for inversed one-to-one relation (v2.3.1) #749

Open
florian-bailly opened this issue Dec 15, 2024 · 16 comments · Fixed by #755 · May be fixed by #775
Open

Null value for inversed one-to-one relation (v2.3.1) #749

florian-bailly opened this issue Dec 15, 2024 · 16 comments · Fixed by #755 · May be fixed by #775
Labels
bug Something isn't working

Comments

@florian-bailly
Copy link

I'm upgrading Foundry on my project to use the v2.

However, it seems like the inversed relation is not auto filled, resulting in a null value (and violating the constraint on insertion), which was not the case before.

I thought this issue would fix ours but it's not the case (since the latest version including the fix is not working).

For the reproducer, I created an Account and AccountSetting entity.

When trying to create an Account with a specified AccountSetting, the account property of the accountSetting is not filled (it is null).

\App\Factory\AccountFactory::createOne([
  'accountSetting' => \App\Factory\AccountSettingFactory::new(),
]);

Link to the reproducer test

@florian-bailly
Copy link
Author

florian-bailly commented Dec 16, 2024

@nikophil on the dev branch, I get a duplicate key for my reproducer test BUT still a null value on project itself.

I can follow up the linked issue if you prefer, and retest when a PR is available :)

@nikophil
Copy link
Member

nikophil commented Dec 16, 2024

I get a duplicate key for my reproducer test BUT still a null value on project itself.

Yeah, that's what I saw from your reproducer.

I've finally found a viable solution for this "one to one inversed" problem, just need to polish things up :)
And I need to find why your case was not covered by our tests suite 🤔

I can follow up the linked issue if you prefer, and retest when a PR is available :)

Thanks, I'd rather to keep both issues open until I'm not 100% sure they are duplicates (we'll see if the patch fixes both issues!)

@nikophil nikophil added the bug Something isn't working label Dec 16, 2024
@nikophil
Copy link
Member

@florian-bailly PR #755 fixes your issue, I just verified with your reproducer :)

@florian-bailly
Copy link
Author

@nikophil indeed it's fixed for the reproducer case :)

However, I still have the null issue on my project on another entity. I'll try to make a reproducer for that case.

@nikophil
Copy link
Member

nikophil commented Dec 17, 2024

yeah, if you can upgrade your reproducer that would be perfect :)

@javiereguiluz
Copy link
Contributor

@nikophil in the docs, you show examples like this:

$post = PostFactory::createOne(); // instance of Proxy

CommentFactory::createOne(['post' => $post]);

But what I want is to use just this:

PostFactory::createOne();

And, inside the PostFactory, use something like this:

protected function defaults(): array|callable
{
    return [
        // ...
        'comments' => [CommentFactory::createOne()],
    ];
}

This is not working as expected and in the error message I see something about null values for non-nullable fields.

I was going to create a new issue ... but then I found this issue and I don't know if it's the same issue reported here or not. Thanks!

@nikophil
Copy link
Member

@javiereguiluz is the project where you have the problem open source? I'd really like to tackle this problem. More over your problem is about OneToMany relationship, it's been the first time I hear of a problem with this kind of relation since a long time 🤔

@florian-bailly
Copy link
Author

@nikophil I think you have fixed the issue regarding the 1:1 since the issues I have are not for the same kind of relations.

I've updated the reproducer (sorry for the entity names, they are poorly chosen).

The error you should have is:

An exception occurred while executing a query: SQLSTATE[08P01]: <>: 7 ERROR: bind message supplies 1 parameters, but prepared statement "" requires 2

and using xdebug I see that on the accountMultipleSettings table, the foo relation is not set... though in the test it's set explicitly.
Screenshot 2024-12-24 at 14 49 40

Also, I'm wondering why the Foo entity is persisted twice. Shouldn't the FooFactory::new() in the AccountFactory defaults be overwritten by the attributes I set in the test?

Finally, how those regressions where possible between the v1 and the v2. Are there missing tests?

@nikophil
Copy link
Member

I've updated the reproducer

thanks, that will be super helpful

I'm wondering why the Foo entity is persisted twice. Shouldn't the FooFactory::new() in the AccountFactory defaults be overwritten by the attributes I set in the test?

I still don't know, I have to deep dive into it 😬

how those regressions where possible between the v1 and the v2. Are there missing tests?

yeah, there are literally tons of different use cases in the wild, and Foundry v2 got a nearly full rewrite
but our testsuite gets more and more robust these days 💪 - by the way, I need to manage to reproduce your problem in Foundry's testsuite 😁

@nikophil
Copy link
Member

nikophil commented Jan 2, 2025

hi @florian-bailly

this was challenging, but I finally tackled your problem!

By the way, still related to inversed one-to-one... I don't know how Foundry 1 was actually better shaped for this kind of things 🤷

@florian-bailly
Copy link
Author

florian-bailly commented Jan 3, 2025

hi @nikophil ,

Well done!

I've tested the fix in my project, but I get the following error:

Doctrine\ORM\ORMInvalidArgumentException: A new entity was found through the relationship 'Api\Entity\AccountSetting#account' that was not configured to cascade persist operations for entity: Api\Entity\Account@2273.
To solve this issue: Either explicitly call EntityManager#persist() on this unknown entity or configure cascade persist this association in the mapping for example @manytoone(..,cascade={"persist"}).

Let me know if it's normal as the v2 is more strict or if you think it's a bug.

@nikophil
Copy link
Member

nikophil commented Jan 3, 2025

gosh, this is a never ending story 😞

I cannot know whether the problem comes from Foundry or your code base only based on this error...

The error is strange, because every entity created by Foundry is given to $em->persist(). But I cannot tell without any hint to reproduce the problem 🤷

@florian-bailly
Copy link
Author

florian-bailly commented Jan 4, 2025

@nikophil I managed to reproduce the error but after cleaning some files to make a simple reproducer, I came accross another error and I can't go back.

I updated the reproducer, hopefully after this last is fixed I'll be able to get back the one we're looking for :)

@nikophil
Copy link
Member

nikophil commented Jan 4, 2025

hi @florian-bailly

did you try this PR, which is not yet merged? it is the one which fixed your previous problem. Using it solves the problem in your reproducer :)

@florian-bailly
Copy link
Author

florian-bailly commented Jan 5, 2025

@nikophil That's weird because I was on the PR 775 but it looks like a change was added after my reproducer.

Anyway, I confirm the case of the reproducer no 3 is fixed. BUT I still have the same error for another case (seems again a 1:1 with other relations).

I'll try to make a reproducer ;)

@nikophil
Copy link
Member

nikophil commented Jan 5, 2025

Yeah, there were some changes on the PR indeed, but before I saw your new reproducer 😅
But I asked because the reproducer did not require the right branch

Thanks for your help, though, very much appreciated 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
3 participants