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

fix(core): quote custom type aliases #1415

Merged
merged 2 commits into from
Feb 11, 2021

Conversation

jbmikk
Copy link
Contributor

@jbmikk jbmikk commented Feb 11, 2021

My custom properties are not getting mapped well because in Postgres casing is ignored for identifiers unless quoted.
The mapper uses the alias to map the results back to the entity and the results are getting lost because it's all lower case.

In other words, the compiled mapping function fails:

if ('my_custom_prop' in result) { ret.myCustomProp = result.my_custom_prop; mapped.my_custom_prop = true; 

Because result.mycustomprop is not matched against the rule, and the generic treatment at the end of the mapping function also fails because it mappes the value into a property that doesn't exist:

for (let k in result) { if (result.hasOwnProperty(k) && !mapped[k]) ret[k] = result[k]; }

I added the plaform quotes for the custom type alias.

@B4nan
Copy link
Member

B4nan commented Feb 11, 2021

Thanks, can you also fix the failing test?

Summary of all failing tests
FAIL tests/custom-types.test.ts
  ● custom types [mysql] › advanced custom types

    expect(received).toMatch(expected)

    Expected substring: "select `e0`.*, ST_AsText(`e0`.point) as point from `location` as `e0` where `e0`.`id` = ? limit ?"
    Received string:    "[query] select `e0`.*, ST_AsText(`e0`.point) as `point` from `location` as `e0` where `e0`.`id` = ? limit ? [took 2 ms]"

      88 |     expect(mock.mock.calls[1][0]).toMatch('insert into `location` (`point`) values (ST_PointFromText(\'point(1.23 4.56)\'))');
      89 |     expect(mock.mock.calls[2][0]).toMatch('commit');
    > 90 |     expect(mock.mock.calls[3][0]).toMatch('select `e0`.*, ST_AsText(`e0`.point) as point from `location` as `e0` where `e0`.`id` = ? limit ?');
         |                                   ^
      91 |     expect(mock.mock.calls).toHaveLength(4);
      92 |     await orm.em.flush(); // ensure we do not fire queries when nothing changed
      93 |     expect(mock.mock.calls).toHaveLength(4);

      at Object.test (tests/custom-types.test.ts:90:35)

Even better would be to make it work so it generates this instead:

ST_AsText(`e0`.`point`) as `point`

@jbmikk
Copy link
Contributor Author

jbmikk commented Feb 11, 2021

I tried running the tests but I could not get them to work.
I'll try later. I just don't have enough time right now.

@B4nan
Copy link
Member

B4nan commented Feb 11, 2021

I guess you saw the contributing guide? Running tests should be quite easy, just run docker-compose and yarn run-rs for mongo.

Will be happy to merge this, if you say it is enough, but I'd expect we need to quote it inside the function too (as noted above).

edit: but this should be enough for the issue you had with mapping the results...

@B4nan B4nan changed the title Add identifier quotes for custom type aliases fix(core): quote custom type aliases Feb 11, 2021
@jbmikk
Copy link
Contributor Author

jbmikk commented Feb 11, 2021

Yes, I read the guide, it said the yarn run-rs was for windows and I didn't run it.
I have run it now and they work.

I'm trying to fix the other quotes now if you wait I may be able to get that in too.

@B4nan
Copy link
Member

B4nan commented Feb 11, 2021

Yes, I read the guide, it said the yarn run-rs was for windows and I didn't run it.

Nope, you misread it. Here is what it says:

Fork the project, install NPM dependencies and start docker to have all databases ready. run-rs is used to manage mongodb replica set. On windows you might need to execute run-rs directly as well as adjust some configuration, refer to their docs for more information.

There is just a note that on windows you should check their docs as it might be more complicated then just running the command.

Maybe I should rephrase that a bit (move the note about windows to a side note under the code snippet maybe).

@jbmikk
Copy link
Contributor Author

jbmikk commented Feb 11, 2021

Yes, I misread it. It was 1 am I should have gone to sleep instead.
Maybe you could add a note below for windows users and just don't explain too much in the main text, just say "do this".

@jbmikk
Copy link
Contributor Author

jbmikk commented Feb 11, 2021

Ok, I tried prefixing the field names but I'm getting a lot of broken tests with double and triple quoted identifiers, and now I realize I'm even getting quoted asterisks.

I think there's just too many particular cases and I wouldn't be comfortable pushing this even with tests passing.

I would like to help getting that fixed but it requires more work, if you think this PR is good enough to merge it would solve at least some problems.

@B4nan
Copy link
Member

B4nan commented Feb 11, 2021

Sure, as said, for the issue you had this should be enough. Thanks anyway :]

@B4nan B4nan merged commit 6f6d1ec into mikro-orm:master Feb 11, 2021
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.

None yet

2 participants