object style where input types by olup · Pull Request #13 · vinpac/nexus-typeorm-plugin

olup · 2020-01-30T10:48:15Z

This is a basic implementation of what I raised in my issue. Seems to work alright, but I did not check the tests.

vinpac

Hey, thanks for your pull request. It's great to have someone willing to help.

I wrote some reviews on things I think we could improve. I miss the checks to see how it goes with tests, but I imagine some of them probably broke since it's currently written on the old format. Could you help me updating the tests and making them pass with this new architecture?

CircleCi didn't run checks on this pull because it comes from a fork. I checked the option to do this on the project settings, so your next commit will trigger the checks. Yay!

Thanks again.

vinpac · 2020-01-30T12:56:18Z

+const whereStringInputType = inputObjectType({
+  name: 'whereStringInputType',


I think StringFilter will be more clear.

const stringFilterInputType = inputObjectType({ name: 'StringFilter',

vinpac · 2020-01-30T12:56:51Z

+const whereIntInputType = inputObjectType({
+  name: 'whereIntInputType',


I think IntFilter will be more clear.

const intFilterInputType = inputObjectType({ name: 'IntFilter',

can we add "Input" nontheless ?

vinpac · 2020-01-30T12:59:35Z

+const whereIntInputType = inputObjectType({
+  name: 'whereIntInputType',
+  definition(t) {
+    t.int('eq')


I think equals makes more verbose queries.

{ post (where: { id: { equals: 1 } } ) }

Rather than

{ post (where: { id: { eq: 1 } } ) }

vinpac · 2020-01-30T13:06:35Z

-                t.field(`${column.propertyName}_${singleOperandOperation}`, {
-                  type: columnTypeName!,
+                t.field(column.propertyName, {
+                  type: whereStringInputType!,


I didn't know we could pass the direct object to the type. To mantain a pattern for now. What do you think of including the inputObject at the src/plugin.ts:42?

return [ buildEntityFieldOutputMethod(manager), buildCRUDOutputMethod(manager), buildEntityOutputProperty(manager), buildEntityFieldsOutputMethod(manager), buildCRUDOutputProperty(manager), GraphQLDateTime, StringFilterInput, IntFilterInput, ]

Suggested change

type: whereStringInputType!,

type: 'StringFilter'

Ok, no problem, it's your lib. But generally speaking I prefer using the object directly as It boast better type support and is less error prone.

Didn't get your last comment about plugin.ts ? Do you want to initiate the input types from this file ? (I actually don't know much about nexus-graphql plugin dev)

Yeah. Os this part we return an array defining the field creators and global types (e.g. GraphQLDateTime). Let's put StringFilter and IntFilter there for now so we don't have multiple implementations on how we are managing types. If later we find it better using the object directly, we migrate it.

I'm looking for reaching a stable version by the end of February.

Not sure how to fix tests tho. I have to regenarate nexus types in the test forlder, and snapshot as well

Is your test suit supposed to pass as is ?

No, you have to adapt the tests. Let me take one test as example.

it('handles simple operations', async () => { const result = await query(` query { users(where: { - age_gt: 35, - age_lt: 50 + . age: { gt: 35, lt: 50 } }) { id name age } }`) expect(result).toMatchObject({ users: [ { age: 40, id: expect.any(String), name: 'baz', }, ], }) })

No, I know 😉 ! I was refering to without my changes. Got almost all test running now, have a few things to work out yet tomorrow. Implementation wise, should I declare my input types in the plugin.ts file or set it up in another file like genericTypes.ts ? Because I also added an "id" input type (only equals and in) and more will come with adaptation of the sorting, and future relay-style pagination.

Wow! Nice work. For now, let's declare it on src/args/arg-where.

olup · 2020-01-31T14:52:27Z

I am also adding the object structure for orderBy. Should those be declared in arg-order ? Looking at nexus-prisma the where type on a single object are different than the one for a find operation. On a single object, only index fields can be queried, without options. What do you think ?

…

On Fri, Jan 31, 2020 at 3:34 PM Vinicius Pacheco Furtado < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/entity-type-def-manager.ts <#13 (comment)> : > @@ -349,29 +367,17 @@ export class EntityTypeDefManager { ) as Nexus.core.AllInputTypes if (columnTypeName === 'String') { singleOperandOperations.forEach(singleOperandOperation => { - t.field(`${column.propertyName}_${singleOperandOperation}`, { - type: columnTypeName!, + t.field(column.propertyName, { + type: whereStringInputType!, Wow! Nice work. For now, let's declare it on src/args/arg-where. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#13?email_source=notifications&email_token=ADJFT5EWY3VJ2N36PUXT6ZDRAQZIBA5CNFSM4KNTXHJ2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCT2CF5I#discussion_r373509256>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADJFT5BJLE6WWVRAXJ7PPHTRAQZIBANCNFSM4KNTXHJQ> .

olup · 2020-01-31T16:07:22Z

Also, I am proposing to remove orderBy from the single find object. If just one is returned, there is no need for ordering. On Fri, Jan 31, 2020 at 3:52 PM Loup Topalian <loup.topalian@gmail.com> wrote:

…

I am also adding the object structure for orderBy. Should those be declared in arg-order ? Looking at nexus-prisma the where type on a single object are different than the one for a find operation. On a single object, only index fields can be queried, without options. What do you think ? On Fri, Jan 31, 2020 at 3:34 PM Vinicius Pacheco Furtado < ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In src/entity-type-def-manager.ts > <#13 (comment)> > : > > > @@ -349,29 +367,17 @@ export class EntityTypeDefManager { > ) as Nexus.core.AllInputTypes > if (columnTypeName === 'String') { > singleOperandOperations.forEach(singleOperandOperation => { > - t.field(`${column.propertyName}_${singleOperandOperation}`, { > - type: columnTypeName!, > + t.field(column.propertyName, { > + type: whereStringInputType!, > > Wow! Nice work. For now, let's declare it on src/args/arg-where. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#13?email_source=notifications&email_token=ADJFT5EWY3VJ2N36PUXT6ZDRAQZIBA5CNFSM4KNTXHJ2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCT2CF5I#discussion_r373509256>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ADJFT5BJLE6WWVRAXJ7PPHTRAQZIBANCNFSM4KNTXHJQ> > . >

olup · 2020-02-01T08:25:14Z

So, following on this, may I change the tests, for example the on that connect a user to a new post. We should be able to connect by name only if the name column on user is Unique ?

Or maybe, just set the name column on user as @Index({unique : true})

olup · 2020-02-01T14:04:05Z

Ok, the pull request is good. I do not know why, one snapshot test doesn't seem to pass on the CI, but works fine on my end.

I had to comment out the query complexity test in complex.test.ts. I do not know how to fix it, maybe you can help me with this.

I apologize for such a big PR. I changed mainly those things :

turned to object-style arguments on where input for the find many query
turned to object-style arguments on orderBy input for the find many query
removed orderBy on find one query. To me, this made no sense at all, tell me if you agree.
added a spedial where argument on the find one query, that takes only fields that identifies the ressource as a single object, ie : ID and any @Index({unique : true}) column

Object style orderBy clause breaks choosing precedence of a multiple ordering query, as JS keys are not ordered (or at least not ordered as the coder types them). We can brainstorm a solution if you accept the PR.

Tell me what you think.

olup · 2020-02-02T09:51:27Z

On the OrderBy front : why not have an "AND" or "THEN" option in the clause so that we can chose hierarchy of ordering ? Like this : orderBy : {age : ASC, AND : { name : DESC }}

vinpac · 2020-02-03T13:33:10Z

Hey @olup ! Nice pull request. I will have more time to review more closely later today or tomorrow. I loved the changes. I'm testing some cases to see if the orderBy and the now unique findOne fall short on some cases.

On the orderBy issue, I think it's important to have a form to control the order of ordering. If we can guarantee that { id: ASC, age: DESC } follows the key definition's order, for me, it's all good. What do you think? It didn't understand much the diference from this approach and the "AND" or "THEN" inside orderBy. Could you explain me?

Let's close on what we have now and discusss so the PR doesn't get too big and points to many directions, making it more dificult to merge. It's better if we divide other concerns and improvements on other PR's. Again, nice work on this PR it's a big help to this project.

olup · 2020-02-03T13:39:47Z

Cheers !
So, to ellaborate on orderBy :
Javascript reorders object's keys in it's own way, so you have NO WAY to keep the order your user decided. If you look at nexus-prisma they don't even tackle multi columns sorting. They just don't care, and it's true that the use-case is pretty minimal.
But if you want to offer it nonetheless, you have to use some kind of organisational system. Either use an array like this [{name : ASC},{age : DESC}]
or have a special key (could be called AND ) that let user chain sortable options in order. Like this : {name : ASC, AND : {age : DESC}}. With this we can insure that first criterium is name and second is age

olup · 2020-02-04T08:32:53Z

Should I add this to my PR ?

vinpac · 2020-02-04T13:34:03Z

Hmm, I feel like the object-style is a nice improvement to where, but for orderBy it seems that an array of strings fits better rather than an array of objects.

orderBy: [age_DESC, name_ASC]

insteadOf

orderBy: [{ age: DESC}, { name: ASC }]

An object-style here allows users to use

orderBy: [{ age: DESC, name: DESC }, { name: ASC }]

It feels strange to read this.

I was thinking about this and came up with an idea. I don't know how I feel about it yet.

order: {
  by: age,
  sort: ASC,
  then: {
    by: name,
    sort: DESC
  }
}

Another example in one line

{ order: { by: age, then: { by: name, sort: DESC } } } 
# Compared to
{ orderBy: [age_ASC, name_DESC] }

1. { orderBy: [age_ASC, name_DESC, updatedAt_DESC] } 
2. { orderBy: [{age: ASC}, { name: DESC}, { updatedAt: DESC }] } 
3. { order: { by: age, sort: DESC, then: { by: name, sort: DESC, then: { by: updatedAt, sort: DESC } } } } 

# Django-style
"age,-name,-updatedAt"

What do you think?

It's more verbose, but can become a large object quickly. An array would do the same with less code. [age_DESC, name_ASC].

Let's keep the orderBy the same way it was and discuss it in another issue/PR? So we don't grow this PR.

I will review it today.

olup · 2020-02-04T13:40:21Z

Ooops ! just published the AND clause. OK, I can revert it, but i really don't like having those double underscore properties. Ok, we can discuss it somewhere else.

olup · 2020-02-04T13:59:20Z

ok, just reverted. Please be careful of the complex test, I had to comment out the last test, I need your input or correction on this.

vinpac · 2020-02-04T18:36:23Z

Yeah, those double underscores seem strange to me too, but I think this is will be a decision with win and losses. I think I've pointed fair points about query size on my previous response, but let's keep discussing it. I think we can reach an agreement about the right way to go with orderBy.

I reviewed the WhereUniqueInput and improved a little bit to include @Unique and @Column({ unique: true }) columns, but I am thinking about how can we cover multiple columns indexes.

For example:
We have the entity UserLikesPost that has a primary key id and one unique index that combines userId and postId. Thus we could find an unique instance by querying { id: <pk> } or { userId: <userId>, postId: <postId> }.

To enable this, we would have to declare a union input type for *WhereUniqueInput.

type UserLikesPostWhereUniqueInput =
  | UserLikesPostWhereUniqueInputPK
  | UserLikesPostWhereUniqueInput_UniqueSetUserIdAndPostId

type UserLikesPostWhereUniqueInput_UniqueSetUserIdAndPostId = {
  userId: Int!
  postId: Int!
}

Sadly union types on GraphQL are still an RFC (https://github.com/graphql/graphql-spec/blob/master/rfcs/InputUnion.md).

I think the solution, for now, is checking if all columns are inside one unique index and make *WhereUniqueInput have all properties as required. Otherwise, we only provide the Primary key on *WhereUniqueInput.

Don't worry about this on your PR. The changes I'm working on also include the WhereUniqueInput. I was already implementing it so will be no problem. I will solve this when I merge.

olup · 2020-02-04T20:02:51Z

Ok, You probably saw that my solution already involves multi indexes and both unique indexes as well as column({unique : true}) ;-)

…

On Tue, Feb 4, 2020 at 7:36 PM Vinicius Pacheco Furtado < ***@***.***> wrote: Yeah, those double underscores seem strange to me too, but I think this is will be a decision with win and losses. I think I've pointed fair points about query size on my previous response, but let's keep discussing it. I think we can reach an agreement about the right way to go with orderBy. I reviewed the WhereUniqueInput and improved a little bit to include @unique and @column({ unique: true }) columns, but I am thinking about how can we cover multiple columns indexes. For example: We have the entity UserLikesPost that has a primary key id and one unique index that combines userId and postId. Thus we could find an unique instance by querying { id: <pk> } or { userId: <userId>, postId: <postId> }. To enable this, we would have to declare a union input type for *WhereUniqueInput. type UserLikesPostWhereUniqueInput = | UserLikesPostWhereUniqueInputPK | UserLikesPostWhereUniqueInput_UniqueSetUserIdAndPostId type UserLikesPostWhereUniqueInput_UniqueSetUserIdAndPostId = { userId: Int! postId: Int! } Sadly union types on GraphQL are still an RFC ( https://github.com/graphql/graphql-spec/blob/master/rfcs/InputUnion.md). I think the solution, for now, is checking if all columns are inside one unique index and make *WhereUniqueInput have all properties as required. Otherwise, we only provide the Primary key on *WhereUniqueInput. *Don't worry about this on your PR.* The changes I'm working on also include the WhereUniqueInput. I was already implementing it so will be no problem. I will solve this when I merge. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13?email_source=notifications&email_token=ADJFT5GXQR4QHMWF3JBQV6DRBGYSPA5CNFSM4KNTXHJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKYWYLA#issuecomment-582052908>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADJFT5CSYFV5QO67Z7SGXODRBGYSPANCNFSM4KNTXHJQ> .

olup · 2020-02-04T20:05:06Z

+    const uniqueIndices = [
+      indices
+        .filter(index => index.isUnique)
+        .map(index => index.columns.map(column => column.propertyName)),
+      uniques.map(unique => unique.columns.map(column => column.propertyName)),
+    ].flat(10)


The comment is misleading : this allows for all unique indexes (even multi columns) AND @column({unique : true})

object style where input types

e09d2e5

vinpac requested changes Jan 30, 2020

View reviewed changes

olup force-pushed the master branch 2 times, most recently from a4343a0 to d99b9f8 Compare January 30, 2020 15:02

change variable names

86aedb5

olup force-pushed the master branch from d99b9f8 to 86aedb5 Compare January 30, 2020 15:21

olup force-pushed the master branch from c9ebb79 to a68551f Compare February 1, 2020 13:17

order arguments + where unique + test fix

bd701bd

olup force-pushed the master branch from a68551f to bd701bd Compare February 1, 2020 13:21

loup topalian added 2 commits February 1, 2020 16:49

fix postgres Test error

9eb66b3

Catch all unique column, rather than just indices

2c8485b

fix test

33ef589

olup commented Feb 4, 2020

View reviewed changes

		const whereStringInputType = inputObjectType({
		name: 'whereStringInputType',

		const whereIntInputType = inputObjectType({
		name: 'whereIntInputType',

Conversation

olup commented Jan 30, 2020

Uh oh!

vinpac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olup Jan 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olup Jan 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vinpac Jan 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olup commented Jan 31, 2020 via email

Uh oh!

olup commented Jan 31, 2020 via email

Uh oh!

olup commented Feb 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olup commented Feb 1, 2020

Uh oh!

olup commented Feb 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vinpac commented Feb 3, 2020

Uh oh!

olup commented Feb 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olup commented Feb 4, 2020

Uh oh!

vinpac commented Feb 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olup commented Feb 4, 2020

Uh oh!

olup commented Feb 4, 2020

Uh oh!

vinpac commented Feb 4, 2020

Uh oh!

olup commented Feb 4, 2020 via email

Uh oh!

olup Feb 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

olup Jan 30, 2020 •

edited

Loading

olup Jan 30, 2020 •

edited

Loading

vinpac Jan 30, 2020 •

edited

Loading

olup commented Feb 1, 2020 •

edited

Loading

olup commented Feb 2, 2020 •

edited

Loading

olup commented Feb 3, 2020 •

edited

Loading

vinpac commented Feb 4, 2020 •

edited

Loading

olup Feb 4, 2020 •

edited

Loading