Bug on joins with null foreign-key

[efraimmgon]

** [EDIT] In the next comments I have identified the issue.

I have a location and a neighborhood table, with the following floor-plan:

{:idents {:locations/all "location"
          :neighborhood/by-id :neighborhood/id}
 :join [:location/neighborhood [:location/neighborhood-id :neighborhood/id]]}

When I query

[{[:locations/all] [{:location/neighborhood [:neighborhood/id]}]}]

I get the following sql output

[SELECT ("location"."neighborhood_id") AS "location/neighborhood-id" FROM "location"]
[SELECT * FROM (SELECT ("neighborhood"."id") AS "neighborhood/id" FROM "neighborhood" WHERE (("neighborhood"."id")=(1)))
 UNION ALL
 SELECT * FROM (SELECT ("neighborhood"."id") AS "neighborhood/id" FROM "neighborhood" WHERE (("neighborhood"."id")=()))]

and the error Error: near ")": syntax error.

You can notice that on the last select the neighborhood.id is not given.

Oddly enough, if I change the query to the following (where I query by id, instead of querying all rows) there's no error:

[{[:location/by-id 1] [{:location/neighborhood [:neighborhood/id]}]}]
--> {[:location/by-id 1] {:location/neighborhood [{:neighborhood/id 1}]}}

I have read the docs and as far as I can tell my floor-plan seems correct.

[efraimmgon]

After a bit more of digging It seems the reason locations/allquery does not work is because one of the values for location.neighborhood_id (foreign key) is null.

I guess the solution would be to filter the rows where the location.neighborhood_id is not nil, but what is the equivalent in walkable since the following seems to be invalid?

[{(:locations/all
   {:filters [:is-not :location/neighborhood-id nil]}) 
  [{:location/neighborhood [:neighborhood/id]}]}]

[efraimmgon]

Indeed, checking the source there's no implementation for :is or :is-not in walkable.sql-query-builder.expressions. It doesn't seem too hard to implement them, but wouldn't it make sense to have them by default? I can make a PR if the author thinks so.

[efraimmgon]

As to the original problem, I think it is a bug and one should not have to filter out the nil foreign keys, simply because you don't have to do it when writing the query in plain sql (especially when you consider you would have to do it for each join table - having done it a couple of times already I can say it's pretty annoying). [EDIT: this does not work because you can only use filters for the main table...]

Of course I have no idea how hard it would be to implement that.

[myguidingstar]

Thanks for reporting the issue and the great analysis. IIRC, SQL is operator has only one use case which is to check with null, so I'd prefer a Clojure-ish operator nil? that generates sql string is null. Combine it with :not like this [:not [:nil? :foo/bar]].

You can implement such nil? operator in walkable by modifying the macrowalkable.sql-query-builder.expressions/import-functions so that it can produce "postfix" functions when arity number is 1, something like:

(import-functions {:arity 1 :postfix? true}
  {nil? " is null"})

Places to look at: https://github.com/walkable-server/walkable/blob/a8fc4bb7f7e6af8b6d7b60c28df3458d5890674a/src/walkable/sql_query_builder/expressions.cljc#L285 https://github.com/walkable-server/walkable/blob/a8fc4bb7f7e6af8b6d7b60c28df3458d5890674a/src/walkable/sql_query_builder/expressions.cljc#L302 https://github.com/walkable-server/walkable/blob/a8fc4bb7f7e6af8b6d7b60c28df3458d5890674a/src/walkable/sql_query_builder/expressions.cljc#L322

[myguidingstar]

Maybe the quickiest fix is to change the following for to filter out cases with v equals nil: https://github.com/walkable-server/walkable/blob/a8fc4bb7f7e6af8b6d7b60c28df3458d5890674a/src/walkable/sql_query_builder.cljc#L474

[myguidingstar]

@efraimmgon can you check if the branch non-int-join fixes the problem?

[efraimmgon]

How do I do that? I'm a git noob, sorry.

[myguidingstar]

@efraimmgon the latest snapshot should work now