cerebris / jsonapi-resources

A resource-focused Rails library for developing JSON:API compliant servers.
http://jsonapi-resources.com
MIT License
2.32k stars 529 forks source link

PG::SyntaxError with Apartment gem #1316

Open cedm opened 4 years ago

cedm commented 4 years ago

This issue is a (choose one):

Checklist before submitting:

Description

Bug reports:

When using Apartment (multi-tenancy gem), any model that is shared among all tenants (i.e. in the 'public' schema) return an error 500 (PG::SyntaxError) when trying to query the API. This issue only happens with shared models, and not with tenant-specific models. The issue lies in the way jsonapi-resources writes its queries, e.g:

Internal Server Error: PG::SyntaxError: ERROR:  syntax error at or near "."
LINE 1: SELECT public.accounts.id AS public.accounts_id FROM "public...

The whole query is:

SELECT public.accounts.id AS public.accounts_id FROM "public"."accounts" WHERE "public"."accounts" = $1

SELECT public.accounts.id AS public.accounts_id FROM \"public\".\"accounts\" WHERE \"public\".\"accounts\" = $1

The bold part is the issue. There should be no dot in the AS statement. public.accounts_id should be public_accounts_id or just accounts_id.

This should be a trivial fix, but I just don't know where in the gem code queries are generated.

cedm commented 4 years ago

Just noticed this is the same issue reported 20 days ago by Albertjan90: https://github.com/cerebris/jsonapi-resources/issues/1310

But no one replied to his report.

cedm commented 4 years ago

Besides the AS statement, the WHERE clause is also erroneous as it is missing the column name:

SELECT public.accounts.id AS public.accounts_id FROM "public"."accounts" WHERE "public"."accounts" = $1

The proper query should be:

SELECT public.accounts.id AS accounts_id FROM public.accounts WHERE public.accounts.id = $1
cedm commented 4 years ago

I've got a solution by modifying the _table_name method found in lib/jsonapi/basic_resource.rb with:

def _table_name
  @_table_name ||= _model_class.respond_to?(:table_name) ? _model_class.table_name.partition('.').last : _model_name.tableize
end

.partition('.').last gets rid of the schema name, e.g. public.accounts -> accounts.

Works well with and without schema for me, but this solution would probably truncate table names if they include a 'dot' (apparently that's allowed in SQL...). Haven't tested the later scenario though.

lgebhardt commented 4 years ago

@cedm Could you test out the #1318 PR and add comments there? I don't have the test framework setup to handle multi tenancy, and unfortunately I'm a bit short on time at the moment to work on that.

Rather than your fix using partition to extract the table portion I'm converting the '.' to '_' to handle the same table name in both databases. Not sure if that's likely to happen, but without it I think we could have issues. Hopefully that doesn't create too long of aliases.

cedm commented 4 years ago

@lgebhardt Your PR fixed the AS statement, but didn't help with the WHERE clause issue. I've commented on the PR.

cedm commented 4 years ago

Improving on my hack-ish solution with:

def _table_name
  @_table_name ||= _model_class.respond_to?(:table_name) ? _model_class.table_name.split('.').last : _model_name.tableize
end

split rather than partition, as partition broke related links.