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

Stored procedure is neither sortable nor filterable, although commented as @sortable and @filterable #1509

Closed
TimoStolz opened this issue Jul 13, 2021 · 4 comments
Labels

Comments

@TimoStolz
Copy link
Contributor

Summary

In GraphiQL, the fields orderBy and condition are missing for the stored procedure search_powerups(varchar) (see below), although it is properly commented as @sortable and @filterable.

Steps to reproduce

#!/usr/bin/env bash
# stolen from https://github.com/graphile/postgraphile/issues/1469#issuecomment-840468985
set -e
DATABASE_NAME="db_9ec0daec_e399_11eb_9008_47882ecae7df"
SCHEMA_NAME="public"

yarn add --dev postgraphile@next
dropdb --if-exists "$DATABASE_NAME"
createdb "$DATABASE_NAME"
psql -X1v ON_ERROR_STOP=1 "$DATABASE_NAME" <<HERE

create extension pg_trgm;

create table powerups
(
    id         uuid primary key default gen_random_uuid(),
    name       varchar not null,
    created_at timestamp default current_timestamp
);

create index powerups_on_fuzzy_name on powerups using gist (name gist_trgm_ops);

insert into powerups (name) values ('cooking'), ('skating'), ('swimming'), ('coding');

---

create type powerup_search_result as (
    similarity real,
    word_similarity real,
    powerup powerups
);

create or replace function search_powerups(by_name varchar)
returns setof powerup_search_result
as \$\$
    select
        similarity(by_name, p.name) as similarity,
        word_similarity(by_name, p.name) as word_similarity,
        p as powerup
    from powerups as p
\$\$ language sql stable strict;

comment on function search_powerups(varchar) is E'@filterable\n@sortable';

HERE

# Recommended options from https://www.graphile.org/postgraphile/usage-cli/
yarn postgraphile \
  --subscriptions \
  --watch \
  --dynamic-json \
  --no-setof-functions-contain-nulls \
  --no-ignore-rbac \
  --show-error-stack=json \
  --extended-errors hint,detail,errcode \
  --export-schema-graphql schema.graphql \
  --graphiql "/" \
  --enhance-graphiql \
  --allow-explain \
  --enable-query-batching \
  --legacy-relations omit \
  --connection "$DATABASE_NAME" \
  --schema "$SCHEMA_NAME"

Expected results

The endpoint searchPowerups should have both an orderBy and a condition field.

Actual results

Both orderBy and condition are missing.

Additional context

All provided above

Possible Solution

Unfortunately, no idea

@TimoStolz
Copy link
Contributor Author

TimoStolz commented Jul 13, 2021

There is a workaround. On discord, @benjie said

it doesn’t have to be a named type, it has to be a selectable (table, view, etc) type. We only generate order/condition for “selectable” types currently: https://github.com/graphile/graphile-engine/blob/v4/packages/graphile-build-pg/src/plugins/PgConnectionArgCondition.js#L21

So I adapted my source and it finally worked.

#!/usr/bin/env bash
# stolen from https://github.com/graphile/postgraphile/issues/1469#issuecomment-840468985
set -e
DATABASE_NAME="db_9ec0daec_e399_11eb_9008_47882ecae7df"
SCHEMA_NAME="public"

yarn add --dev postgraphile@next
dropdb --if-exists "$DATABASE_NAME"
createdb "$DATABASE_NAME"
psql -X1v ON_ERROR_STOP=1 "$DATABASE_NAME" <<HERE

create extension pg_trgm;

create table powerups
(
    id         uuid primary key default gen_random_uuid(),
    name       varchar not null,
    created_at timestamp default current_timestamp
);

create table powerup_search_results (
    similarity real,
    word_similarity real,
    powerup powerups
);

-- instead of
-- create type powerup_search_result as (
--    similarity real,
--    word_similarity real,
--    powerup powerups
-- );

create index powerups_on_fuzzy_name on powerups using gist (name gist_trgm_ops);

insert into powerups (name) values ('cooking'), ('skating'), ('swimming'), ('coding');

---

create or replace function search_powerups(by_name varchar)
returns setof powerup_search_results
as \$\$
    select
        similarity(by_name, p.name) as similarity,
        word_similarity(by_name, p.name) as word_similarity,
        p as powerup
    from powerups as p
\$\$ language sql stable strict;

comment on function search_powerups(varchar) is E'@filterable\n@sortable';

HERE

# Recommended options from https://www.graphile.org/postgraphile/usage-cli/
yarn postgraphile \
  --subscriptions \
  --watch \
  --dynamic-json \
  --no-setof-functions-contain-nulls \
  --no-ignore-rbac \
  --show-error-stack=json \
  --extended-errors hint,detail,errcode \
  --export-schema-graphql schema.graphql \
  --graphiql "/" \
  --enhance-graphiql \
  --allow-explain \
  --enable-query-batching \
  --legacy-relations omit \
  --connection "$DATABASE_NAME" \
  --schema "$SCHEMA_NAME"

@TimoStolz
Copy link
Contributor Author

I would still prefer if it also worked with types. With fake tables, it feels somewhat hacky, and it causes additional care to prevent inserts on them. Probably, there could be a @selectable tag for types.

@benjie
Copy link
Member

benjie commented Jul 14, 2021

I'm not sure if a selectable tag is the right approach, but there should be something. The risk of unilaterally defining the conditions/orders for all types is that we increase risk of name collisions even though they're not necessarily even reaching the final schema. We could try and "guess" if the type is going to be used (e.g. it's the return type of a function that's tagged in the relevant way) but that kinda sucks.

What should really happen is that the function should be able to request the condition/order type for the given type it uses and is planning to put into the schema, and that type is generated just in time if it doesn't already exist. This is the pattern that I've been trying to use for newer code in Graphile but alas it'll be quite a big lift to refactor the existing code to do this I think.

@benjie
Copy link
Member

benjie commented Oct 4, 2023

V5 solves this via the behavior system - we generate the filter types for the codecs that have the filterBy behavior.

@benjie benjie closed this as completed Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

No branches or pull requests

2 participants