if OneToMany entity was updated right before, em.save() does not save entity attached in ManyToOne, resulting in parentId column <null>

[idchlife]

It's very strange. I assume I did nothing wrong.

Here is column:

@ManyToOne(type => RoastAttempt, ra => ra.reactions)
  attempt: RoastAttempt;

here is I'm saving:

const reaction = new RoastAttemptReaction();

reaction.attempt = a;
reaction.type = type;
reaction.user = u;

await this.entityManager.save(reaction);

I did console.log of reaction before saving - entity is there. in the attempt field.

I should also mention .user field on the other hand is populated and saved.

version: "typeorm": "^0.1.7"

edit: em.create() has the same behaviour

[pleerock]

I need a reproduction repo to test your issue, also make sure to check on both 0.1.7 and next

[idchlife]

@pleerock can I add your github public email to private bitbucket repo and share instruction to start app also via email?

[pleerock]

sorry my time is limited to do that, better if you create a PR with a failing test which I can quickly fix.

[idchlife]

np, will try to do that

[idchlife]

Interesting. Actually @next is saving entity nicely! But I will try to find a cause in 0.1.7

I'm afraid that (as always happens to me) repo to reproduce actually will not have this bug.

Trying to figure out this by myself. What I discovered:

Postgres actually getting proper insert statement with proper parameters:

INSERT INTO "roast_attempt_reaction"("type", "createdAt", "updatedAt", "userId", "attemptId") VALUES ($1,DEFAULT,DEFAULT,$2,$3) [ 'upvote', 28, 6 ]

But then, when I update RoastAttempt (just jsonb column value), which has reactions via

reactions: RoastAttemprReaction[];

I'm getting this weird sql statement being send to postgresql (second statement)

UPDATE "roast_attempt" SET "cachedReactions"=$1, "updatedAt"=$2 WHERE "id"=$3 [ '{"upvote":2}', 2017-12-08T07:55:46.179Z, 6 ]
UPDATE "roast_attempt_reaction" SET "attemptId"=$1 WHERE "id"=$2 [ null, 57 ]

[idchlife]

upd, found the solution for now (but not fix for the bug):

It seems that problem somewhere where I'm doing this:

      const reaction = new RoastAttemptReaction();

      reaction.attempt = a;
      reaction.type = type;
      reaction.user = u;

      await this.entityManager.save(reaction);

      a.modifyReaction(type, 1);

      a = await this.entityManager.save(a);

I suspect SubjectOperationExecutor somewhere sees Attempt (which has reactions) having 1 reaction and removes from this 1 reaction id to attempt (attemptId).
I should notice, that this happens even if Reaction is inserted via raw SQL query, thus pointing to place where ORM is reading entity from database and processing it before saving. It founds one "child" entity and for some reason clears it from parent entity.
@next version does not have this, bug though (but I do not suggest using it, since there is already strange bug there with saving jsonb in postgres as string but not jsonb, also reading it as string too)

If I swap code above, first update parent entity and than create new child entity - everything works fine. No additional update to set null.

SOLUTION:
If someone stumbled upon this strange issue, just create ManyToOne child entity after updating OneToMany entity:

      a.modifyReaction(type, 1);

      a = await this.entityManager.save(a);

      const reaction = new RoastAttemptReaction();

      reaction.attempt = a;
      reaction.type = type;
      reaction.user = u;

      await this.entityManager.save(reaction);

[pleerock]

I don't have enough details to reproduce your issue and help, I'll able to help you only if you provide a git repo or better a failing test with minimal reproduction code, and only on @next since there are lot changes in next regarding to how save works, some of fixes we have in 0.2.0 cannot be applied in 0.1.0, so better if we work with 0.2.0

[idchlife]

@pleerock I think since this issue is not present in @next this simple trick would be enough for the moment. If it's possible - I don't think issue should be closed yet, maybe I will have time to dig deeper into the problem later or actually will manage to reproduce it.

The most basic solution works.

[idchlife]

@pleerock it occured that this bug is easy to catch even in clean new project. I made repo to reproduce:

https://github.com/idchlife/typeorm-bug1-repro

Instructions:

npm i

# replace values for postgresql in src/tests.ts

./tests.sh

btw, can I ask you and @19majkel94 , what IDEs are you using to code for typeorm? I opened it in VSCode and it's intellisense was kinda dying every minute.

[MichalLytek]

Yes, I'm using VSCode, I had no problem with intellisense and performance but I noticed that sometimes the whole TypeScript language service hangs and don't check the code nor underline the error, I need to restart it or wait a moment 😕

[pleerock]

You have this error because you have initialized array:

  @OneToMany(type => Child, c => c.parent)
  children: Child[] = [];

Remove = [] and you'll not have this error. Avoid array initialization in your relations, read here for more info.

When typeorm tries to save your parent it sees its children array is empty and thinks you removed all childrens from parent and detaches them.

[idchlife]

@pleerock interesting. This is because by default typeorm would not even touch property "children", because no one told to load those relations, so the value is []

Wonderful! Is there way to warn about this stuff or something? Like "you loaded entity without relations from database and there is property initializer. Maybe check again?"

Thanks for checking, this is definitely gone above my head. I remembered there were some errors with array initializer but just forgot about this caveat.

[pleerock]

so the value is []

value [] means everything was removed, just think how it would work if you wanted to really remove all children from your parent.

Is there way to warn about this stuff or something?

I have no idea how to warn about this thing.

this caveat.

yes this is tricky, unfortunately

[MichalLytek]

I have no idea how to warn about this thing.

When you register entity class in metadata storage (using @Entity), you can make a new object of this class and check if it has some keys - this will mean that there are property initializers.

Of course you can limit this check only to relation fields to prevent false warning, eg. when someone have enum field with default value.

[pleerock]

what kind of warning it shall be? Shall we give an error of simply warning? And what if user wants to initialize property and disables entity updation from inverse side?

[MichalLytek]

And what if user wants to initialize property and disables entity updation from inverse side?

Can we check that too? 😉

Ask yourself a question - is it better to print false warning on console or have this kind of issues and many angry developers because "this shit doesn't work" due to properties initialization?

[pleerock]

so, what do you think is better - give a warning in console or give an error? Or maybe there is some other solution like cli command typeorm doctor which will tell about those issues?

[MichalLytek]

It might be a solution but must be well documented, e.g. in issue template "please paste logs from typeorm doctor", info in readme, etc. 😃

[pleerock]

yeah its kinda safe way, but what is the best way, Im seeking for opinions. Adding @daniel-lang @AlexMesser @NoNameProvided @mrkmg

[mrkmg]

Hmmmm... I think a typeorm doctor command would be cool, but if the only if there is more than one check. From this conversation, all I am seeing is the desire to warn about initializers on relations. Seeing as that's the only case, I think a console.warn is good enough.

If we do emit warnings, we should also provide some sort of "suppressWarnings" flag that allows developers to turn off warnings.

I was thinking, how cool would it be to have tslint, or even typescript give a compile-time warning about it, but after looking through the docs on how types work, I can not see a way to accomplish that, so a runtime warning is the only option I see.

If we can come up with a few checks for best practice, then I can see the use-case of a full command like doctor. Some ideas could be:

• Missing inverse of relationship
• Type MisMatch (saying "integer" db type, but declaring the property as type "string")
• Recursive "eager" checks

That's my two cents.

[mrkmg]

We could just have both as well. Emit warnings to STDERR (or STDOUT), and have the doctor command to pull them all together in a report.

[pleerock]

Thank you for your feedback @mrkmg . I decided not to do doctor since it still will bring user issues because most of them will not ever know about "doctor" command and they will keep throwing issues.

Regarding to warning in console - it will look very weird in my opinion, better to throw an error instead I think. This change will be available in the next next version.

[MichalLytek]

Throwing error is ok but what about false positive? E.g. when user wants to initialize property and disables entity updation from inverse side? Supressing errors would be worse than typeorm doctor.

[daniel-lang]

I'm fine with an error as it seems like @pleerock only limited to ManyToMany and OneToMany relationships.

If the list of things to check gets longer, I would be in favour of a doctor command as it might help with issues and general troubleshooting.