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

Only include global type (pg_catalog) or types in exposed namespaces #99

Closed
wants to merge 1 commit into from

Conversation

pyramation
Copy link

@pyramation pyramation commented Oct 25, 2017

Fixes #100

@benjie
Copy link
Member

benjie commented Oct 25, 2017

This would break backwards compatibility - e.g.

create type secret.foo as (
  bar text
);

create function web.get_foo() returns secret.foo as $$
  select ('Hello')::secret.foo;
$$ language sql stable;

get_foo() should be exposed because it's in the public (web) schema; it depends on private type (not data, just type) so the type needs to be made available.

What's the motivation for this change?

I'm working on something towards this in new (unpushed) commits related to #92 - basically I'm going to filter it in JS rather than in the introspection query because it turns out that even a recursive CTE to achieve this is pretty complex!

@pyramation
Copy link
Author

The motivation comes from creating custom types in isolated schemas. You could create multiple postgraph instances on different schemas of the same database, however, there would be a conflict and also a "leak" of information across schemas.

I see the reason for a CTE, maybe I can look into that -- this would recursively look for all types that are used in a function, returned by a function, used in tables, etc?

@benjie
Copy link
Member

benjie commented Oct 25, 2017

The motivation comes from creating custom types in isolated schemas. You could create multiple postgraph instances on different schemas of the same database, however, there would be a conflict and also a "leak" of information across schemas.

I think filtering this in the JS code will solve it. Basically we'll walk through the public schema and pull in only the types that are actually used in things to be exported (table/view types, column types, return types on functions, etc) and then drop all the others from the introspection results. This will prevent the current conflicts that are happening from us eagerly creating types that turn out to be unused.

Nothing should "leak" (even now) from the private schemas to the public GraphQL API unless it is referenced in the public schema (e.g. as the return type of a function).

@benjie
Copy link
Member

benjie commented Oct 25, 2017

This is how far I got with the recursive query but I had to stop rather abruptly due to a phone call and haven't had time to get back to it yet (I don't think it runs yet):

  -- @see https://www.postgresql.org/docs/9.5/static/catalog-pg-class.html
  class as (
    with recursive klass("kind", "id", "name", "description", "namespaceId", "namespaceName", "typeId", "isSelectable", "isInsertable", "isUpdatable", "isDeletable") as (
    select
      'class' as "kind",
      rel.oid as "id",
      rel.relname as "name",
      dsc.description as "description",
      rel.relnamespace as "namespaceId",
      nsp.nspname as "namespaceName",
      rel.reltype as "typeId",
      -- Here we determine whether or not we can use this class in a
      -- `SELECT`’s `FROM` clause. In order to determine this we look at them
      -- `relkind` column, if it is `i` (index) or `c` (composite), we cannot
      -- select this class. Otherwise we can.
      rel.relkind not in ('i', 'c') as "isSelectable",
      -- Here we are determining whether we can insert/update/delete a class.
      -- This is helpful as it lets us detect non-updatable views and then
      -- exclude them from being inserted/updated/deleted into. For more info
      -- on how `pg_catalog.pg_relation_is_updatable` works:
      --
      -- - https://www.postgresql.org/message-id/CAEZATCV2_qN9P3zbvADwME_TkYf2gR_X2cLQR4R+pqkwxGxqJg@mail.gmail.com
      -- - https://github.com/postgres/postgres/blob/2410a2543e77983dab1f63f48b2adcd23dba994e/src/backend/utils/adt/misc.c#L684
      -- - https://github.com/postgres/postgres/blob/3aff33aa687e47d52f453892498b30ac98a296af/src/backend/rewrite/rewriteHandler.c#L2351
      (pg_catalog.pg_relation_is_updatable(rel.oid, true)::bit(8) & B'00010000') = B'00010000' as "isInsertable",
      (pg_catalog.pg_relation_is_updatable(rel.oid, true)::bit(8) & B'00001000') = B'00001000' as "isUpdatable",
      (pg_catalog.pg_relation_is_updatable(rel.oid, true)::bit(8) & B'00000100') = B'00000100' as "isDeletable"
    from
      pg_catalog.pg_class as rel
      left join pg_catalog.pg_description as dsc on dsc.objoid = rel.oid and dsc.objsubid = 0
      left join pg_catalog.pg_namespace as nsp on nsp.oid = rel.relnamespace
    where
      -- Select classes that are in our namespace, or are referenced in a
      -- procedure or a column
      (
        rel.relnamespace in (select "id" from namespace) or
        rel.reltype in (select "returnTypeId" from procedure) or
        rel.reltype in (select unnest("argTypeIds") from procedure) or
        rel.reltype in (select typrelid from pg_type where typarray in (select unnest("argTypeIds") from procedure))
      ) and
      rel.relpersistence in ('p') and
      -- We don't want classes that will clash with GraphQL (treat them as private)
      rel.relname not like '\_\_%' and
      rel.relkind in ('r', 'v', 'm', 'c', 'f')
    UNION
    select
      'class' as "kind",
      rel.oid as "id",
      rel.relname as "name",
      dsc.description as "description",
      rel.relnamespace as "namespaceId",
      nsp.nspname as "namespaceName",
      rel.reltype as "typeId",
      false as "isSelectable",
      false as "isInsertable",
      false as "isUpdatable",
      false as "isDeletable"
    from
      pg_catalog.pg_class as rel
      inner join klass on (
        klass.id = any ( select attrelid from pg_attribute where atttypid = rel.reltype) or
        klass.id = any (select attrelid from
        klass.id = any (select attrelid from pg_attribute where atttypid = any (select typarray from pg_type where typrelid = rel.reltype))
        klass.id = any (select attrelid from pg_attribute where atttypid = any (select typarray from pg_type where typrelid = rel.reltype))
      )
      left join pg_catalog.pg_description as dsc on dsc.objoid = rel.oid and dsc.objsubid = 0
      left join pg_catalog.pg_namespace as nsp on nsp.oid = rel.relnamespace
    where
      rel.relpersistence in ('p') and
      -- We don't want classes that will clash with GraphQL (treat them as private)
      rel.relname not like '\_\_%' and
      rel.relkind in ('r', 'v', 'm', 'c', 'f')
    ) select * from klass
  ),

I think filtering the original query it in JS will be a LOT easier (and probably faster too!) than writing this recursive CTE correctly; not to mention it'd be easier to maintain!

@benjie
Copy link
Member

benjie commented Oct 28, 2017

@pyramation I'm hoping this is solved by #92; let me know if not and we can revisit.

@benjie benjie closed this Oct 28, 2017
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