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

needs foreign table info in inflector.manyRelationByKeys #77

Closed
mathroc opened this issue Sep 22, 2017 · 4 comments · Fixed by #80
Closed

needs foreign table info in inflector.manyRelationByKeys #77

mathroc opened this issue Sep 22, 2017 · 4 comments · Fixed by #80
Milestone

Comments

@mathroc
Copy link
Contributor

mathroc commented Sep 22, 2017

I’d like to know the foreign table name when building the manyRelationByKeys name

use case, column site on table discussion targets a row in table site

on the type Site I’d like to call the field discussions instead of discussionBySite because there is only one column on the key and that this column is named as the foreign key target table

@benjie
Copy link
Member

benjie commented Sep 22, 2017

I’d like to know the foreign table name when building the manyRelationByKeys name

I believe this is already possible:

      manyRelationByKeys(detailedKeys: Keys, table: string, schema: ?string) {
        return camelCase(
          `${this.pluralize(
            this.tableName(table, schema)
          )}-by-${detailedKeys
            .map(key => this.column(key.column, key.table, key.schema))
            .join("-and-")}`
        );
      },

the table and schema arguments refer to the target table, and the keys contain the source table and schema that the keys come from.

use case, column site on table discussion targets a row in table site

I believe you're saying this:

create table site (
  id serial not null primary key
);

create table discussion (
  id serial not null primary key,
  site int not null references site(id)
)

because there is only one key and that this key is named as the target table, I’d like to call it "site" instead of "discussionsBySite"

On the Discussion type, if set up as above, there should be a field siteBySite which references the remote table site using the local column site. If you rename this to site it will clash with the column that is named site - you may need to combine this, then with the pgColumnFilter that's being introduced in #73 so that the column is never added to the schema. Personally I'd have named the column site_id and then having the relation renamed to site would cause no issues.

(which is weird on its own, it should be sitesByDiscussions, shouldn’t it?)

The relation discussionsBySite would be the reverse relation, found on the Site type, and represents the one-to-many nature of the relationship (one site has many discussions). In this case it is returning a list of discussions (target table, plural) that are identified by site (target table column since it's a reverse relation); hence discussionsBySite.

@benjie
Copy link
Member

benjie commented Sep 22, 2017

the table and schema arguments refer to the target table, and the keys contain the source table and schema that the keys come from.

My bad, they all refer to the foreign table because it's a reverse relation. I'd definitely consider a PR that added the local table and schema as additional arguments to that inflector 👍

@mathroc
Copy link
Contributor Author

mathroc commented Sep 22, 2017

sorry I posted this too fast and made some mistakes / wasn’t very clear.yes, I’m talking about the field added by the PgBackwardRelationPlugin on the Site type.

you’re absolutely right about the schema and the thing about pgColumnFilter that’s exactly why I wanted to filter fields in the first place :)

should the 2nd and 3rd arguments be kept as they are always the same as in the key columns ? (or maybe keys should contain only the column names?) that would be a BC break. tell me and I’ll make a PR

@benjie
Copy link
Member

benjie commented Sep 22, 2017

that’s exactly why I wanted to filter fields in the first place :)

😳 I should spend more time reading people's usernames

should the 2nd and 3rd arguments be kept as they are always the same as in the key columns ? (or maybe keys should contain only the column names?) that would be a BC break. tell me and I’ll make a PR

I think the existing values should be kept because then it mirrors the forward relation; it's annoying to have 5 arguments but ¯\_(ツ)\_/¯

I probably should have gone with a named hash API instead. Oh well 😅

mathroc added a commit to mathroc/graphile-build that referenced this issue Sep 23, 2017
mathroc added a commit to mathroc/graphile-build that referenced this issue Sep 24, 2017
@benjie benjie added this to the 4.0 milestone Aug 16, 2018
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 a pull request may close this issue.

2 participants