Added $leftJoinRelation

[wz5899]

Added $leftJoinRelation. leftJoinRelation is supported by objection.js, but feathers-objection has no corresponding $leftJoinRelation.

[dekelev]

Thanks @wz5899 , I'm not much available to review this in the next few weeks, but I'll check it out eventually.

[TalArbatov]

this seems like a good solution

[dekelev]

Sorry, I haven't got the chance to review it yet and won't be able to do this in the next 2 weeks, but I'll keep a reminder.

[dekelev]

@wz5899 Looks good to me. Please add a test to cover this new feature. Thanks for contributing!

[dekelev]

Thanks @wz5899, I'll let you know once this is released.

[wz5899]

HI, Dekel

I added following changes in my branch (a375d91):

1. Two tests.
2. Reversed 1f78063. The if conditions in my original code are not redundant. They are for the situation that both joinRelation and leftJoinRelation are present in the query. I ran into a situation like this and that's why I added this change. I added a test for this situation. For example, find companies have clients (joinRelation) regardless of the company has or has no employees (leftJoinRelation).

I am having difficulty running the test in my vsc environment. The node-gyp gives build errors all the time for sqlite3. I have been twisting my environment all day but no luck. Can someone run these tests and verify?

Warren

On Mon, Feb 27, 2023 at 4:47 AM Dekel Barzilay @.***> wrote:

Thanks @wz5899 https://github.com/wz5899, I'll let you know once this is released.

— Reply to this email directly, view it on GitHub https://github.com/feathersjs-ecosystem/feathers-objection/pull/182#issuecomment-1446017288, or unsubscribe https://github.com/notifications/unsubscribe-auth/APYGFNLVBIIEQIAJT6HEQ53WZRZZXANCNFSM6AAAAAAQJ5M37Q . You are receiving this because you were mentioned.Message ID: @.***>

[dekelev]

@wz5899 Sure, I plan to test it locally before merging anyway. I'll also check later if I can fix the broken TravisCI integration. Regarding the sqlite3 error, I remember that, but I'm not sure how I worked around it, so if I'll remember or face this issue myself again, I'll update you here.

[dekelev]

Thanks @wz5899, I ran the tests in your PR branch and there are 2 failed tests regarding "Join Eager/Relation queries".

You can run the tests now, just pull updates and install dependencies.

Also please delete the .vscode file from the remote branch. It should not be included here. You can add .vscode to the gitignore file or I'll add it later.

[dekelev]

Thanks @wz5899, It looks good on SQLite, but your test fails on PostgreSQL with a "GeneralError: Unknown Database Error", not sure why.

You can test it out with changing the db variable in the index.test.js file to something like:

const db = knex({
  client: 'pg',
  debug: false,
  connection: {
    host: '127.0.0.1',
    port: 5432,
    user: 'user',
    password: 'password',
    database: 'test'
  },
  useNullAsDefault: false
});

Create the test PG database and run your test.

Note that not all tests are supposed to pass on PG, since some of them only work with SQLite.

[wz5899]

The "GeneralError: Unknown Database Error" in PostgreSQL seems from a bug in Objection or feathers-objection.

The error was caused by a query error: DBError: select distinct "companies".* from "companies" inner join "clients" on "clients"."companyId" = "companies"."id" left join "employees" on "employees"."companyId" = "companies"."id"

For some unknown reason, Objection adds "distinct" before "companies".* in the generated PostgreSQL query. This throws an error. I have to add a groupBy in the query to suppress this distinct.

I saw another existing test "allow $modify query with paginate, groupBy and joinRelation" has the same issue. It uses a groupBy in $modify to work around. Without that groupBy, that test will give the same Unknown Database Error.

[dekelev]

@wz5899 Thanks! it looks great. I saw another change though in an existing test that wasn't mentioned.

Can you please explain this change:

[wz5899]

I added an employee 'E' in ids.employees without an affiliated company in 'before' hook to test leftJoinRelation. That employee 'E' is at index 0 in the employee list fetched in "allows joinEager queries".

[dekelev]

I see, thanks. Great work, I'll merge it later this week.

[dekelev]

@wz5899 Thank you for your contribution! It's now released in v7.6.0.