zenstruck / foundry

A model factory library for creating expressive, auto-completable, on-demand dev/test fixtures with Symfony and Doctrine.
https://symfony.com/bundles/ZenstruckFoundryBundle/current/index.html
MIT License
611 stars 63 forks source link

Possibility of not flushing at all #507

Closed jorismak closed 9 months ago

jorismak commented 9 months ago

Hi

This might be a bit out of scope for Foundry, but anyway...

We would like to use the factories to create objects in normal production as well. We have an async background job that goes over hundreds to thousands of entities and checks if everything exists that needs to be, has the right properties, etc...

Currently , that whole process is split up in a few Symfony services. And there is a single flush at the end.

While I could use 'delayFlush' to batch a few foundry operatuons, is there also a mode to just disable flashing all together and do it manually at the end ?

Or should I just wrap the entry point of that whole process in one delayFlush handler ?

nikophil commented 9 months ago

I'm not sure to really understand what you want to achieve here :thinking:

In Foundry, you can do $this->disablePersist(); which is a method you can find in Factories trait. If this method is used, none of the entities created by Foundry will be persisted/flushed.

nevertheless, it does not affect the rest of the application, and I also actually think this request is out of the scope of this library.

delayFlush() won't affect your application either, because this is a behavior restricted to Foundry.

jorismak commented 9 months ago

I have a long process , with normal entity changes AND foundry work.

I want to flush only once at the end of it all. disablePersist doesn't seem what I want , because I do want persist() to be called on new entities creates by foundry.

I just want to be the flush to be delayed to a point 'far away' from the code actually using foundry. Or in other words , I'd like to manually call entityManager->flush() somewhere in my code instead of foundry doing it for me.

On Thu, Sep 14, 2023, 10:45 Nicolas PHILIPPE @.***> wrote:

I'm not sure to really understand what you want to achieve here 🤔

In Foundry, you can do $this->disablePersist(); which is a method you can find in Factories trait. If this method is used, none of the entities created by Foundry will be persisted/flushed.

nevertheless, it does not affect the rest of the application, and I also actually think this request is out of the scope of this library.

delayFlush() won't affect your application either, because this is a behavior restricted to Foundry.

— Reply to this email directly, view it on GitHub https://github.com/zenstruck/foundry/issues/507#issuecomment-1719022862, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALMD2DQZ2F4L62RJK4WW7Y3X2K7ZPANCNFSM6AAAAAA4XTZ3PE . You are receiving this because you authored the thread.Message ID: @.***>

nikophil commented 9 months ago

ok then, I understand better now!

what you need is some sort of disableFlush().

I'm a bit reluctant to add kind of a method disableFlush() into Foundry, because the only usage I'd see would be in production code, and the main purpose of the library is for testing or eventually dev purposes. Any thoughts @kbond?

By the way, since we're heavily using reflection in Foundry, IMHO usages in production should be avoided because of performance reasons.

what you can do for now would be to enable disablePersist(), which will neither call $em->persist($entity) nor $em->flush(), and persist by yourself the entities.

What I'd do is the following:

final class SomeEntityFactory extends ModelFactory
{
    public function __construct(private EntityManager $em){}

    public function withoutFlush(): static
    {
        return $this->withoutPersist()
            ->afterInstantiate(
                 function(SomeEntity $object): void {
                     $this->em->persist($object);
                 }
            );
    }
}

this way your entities would be automatically persisted but not flushed. You can even add this in a trait, in order to share the method

jorismak commented 9 months ago

Awesome, good reasoning. I'll see it this is viable for us.

(And yes, production code .. but it's a big async background job . Its a long running process with loads of action . Performance of reflection is not an issue. And with automappers and serialization, isn't reflection used in production anyway ?).

nikophil commented 9 months ago

And with automappers and serialization, isn't reflection used in production anyway ?

if you're talking about jane's automapper, it only uses reflection when generating the mappers, which should be done at cache warming phase

for serialization not so sure it uses reflection a lot, or at least it is cached I think

kbond commented 9 months ago

I'm a bit on the fence. Because of the static nature of a possible ::disableFlush()/::flush(). It's possible it could be disabled w/o re-enabling by accident leading to a hard to debug issue. The nature of encapsulating it all in ::delayFlush() prevents this problem.

I'd like to see if you could implement @nikophil's idea or as you said above, wrap the entire process in a ::delayFlush() callback.

jorismak commented 9 months ago

I was already ook with that ! The idea that the library is meant for testing / fixtures makes sense , so keep your scope. I'm the weire one using it for something else :).

kbond commented 9 months ago

Ok, closing this issue as resolved. Just make sure you keep your production use of this package to background jobs! 😆

kbond commented 9 months ago

Hmm, actually, since you're using in production, you have the bundle enabled in production? Even when not using in a background job? This would mean foundry is booted on every request.... and that... could still cause some performance issues - or at least no consideration was made to optimize this.

jorismak commented 9 months ago

We don't have it in production yet.

We have a service that was assisting in creating objects of all kind. And used in generating fixtures to get development data. And in tests. A lot of that has moved to foundry.

It feels wrong to have a piece of custom code that is doing what foundry is basically doing .

I haven't used it with app_env to prod to be honest. Performance wise I didn't think too much of it since basically everything is done in service subscribers or other 'on demand' things... but yeah, maybe foundry has a huge initialization impact on every request 'just being there's. Might be worth checking out 🙈

On Thu, Sep 14, 2023, 17:56 Kevin Bond @.***> wrote:

Hmm, actually, since you're using in production, you have the bundle enabled in production? Even when not using in a background job? This would mean foundry is enabled on every request.... and that... could still cause some performance issues - or at least no consideration was made to optimize this.

— Reply to this email directly, view it on GitHub https://github.com/zenstruck/foundry/issues/507#issuecomment-1719724019, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALMD2DS55MMNMV7U5KWEFDDX2MSJXANCNFSM6AAAAAA4XTZ3PE . You are receiving this because you authored the thread.Message ID: @.***>

jorismak commented 9 months ago

After a quick internal discussion, we decided to ditch Foundry in production because of your concerns. We don't have the time / attention to do proper testing, and if the library is not meant for it, that's reason enough not to try and use it differently :).