maoosi / prisma-appsync

⚡ Turns your ◭ Prisma Schema into a fully-featured GraphQL API, tailored for AWS AppSync.
https://prisma-appsync.vercel.app
BSD 2-Clause "Simplified" License
226 stars 18 forks source link

Try and add support for nested arguments (`where`, `orderBy`, `take`, `skip`) #124

Closed Tenrys closed 1 year ago

Tenrys commented 1 year ago

I also updated Prisma to 4.13.0.

I didn't update tests, and this is probably very rough, but this does work currently for my purposes, so I'm making a pull request in case you want to make this cleaner to your tastes.

I left old unused code commented out, and imported some useful functions from the dev server package for the actual client. I don't know if this is a bad practice, but it does work.

Feel free to ask if you have any questions about the changes I made, I'll be happy to answer them.

Related: https://github.com/maoosi/prisma-appsync/issues/117

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment | Name | Status | Preview | Comments | Updated (UTC) | | :--- | :----- | :------ | :------- | :------ | | **prisma-appsync** | ⬜️ Ignored ([Inspect](https://vercel.com/maoosi/prisma-appsync/GoVbZMVJKQahXjjxJBu825BsrxH9)) | | | Apr 26, 2023 1:08pm |
maoosi commented 1 year ago

Thanks @Tenrys ! Seems like a lot of changes. I'll have a proper look after the 1.0.0-rc.6 release.

maoosi commented 1 year ago

@Tenrys is the PR ready to be reviewed, or still working on it?

Tenrys commented 1 year ago

It should be ready.

Tenrys commented 1 year ago

If you're going to review something, I suggest you look at my fork with the branch I'm working on every time I find something that doesn't work right, it encapsulates all the PRs I have out right now, as such, it's probably better for you to make a judgement based on all those changes at once.

Overall, I tried the best I could to fix fragments (fragment definitions and inline fragments), aliases, __typename on dev server, nullables for use with graphql-codegen, all this under the assumption that I'm using the new way of parsing select I wrote with the help of your graphqlToJson function for the sake of being allowed to use nested arguments like Prisma allows you to.

The only problem is, since I don't do any unit testing, I am far from confident this will all work reliably. I'm using my fork in development which so far doesn't seem to be running into any issues, but it's difficult to know for sure.

Tenrys commented 1 year ago

I've come to the conclusion that I went the wrong way around when trying to add support for this and fragments. I'm going to make a cleaner proposal at some other point.

maoosi commented 1 year ago

@Tenrys curious to know why this conclusion? I liked the idea of using graphqlToJson() instead of manually creating the query objects. The only thing I am not sure about with that approach is compatibility with JSON fields.

Tenrys commented 1 year ago

I believe that doing it this way made me go down the rabbit hole of trying to re-implement fragments, aliases, etc manually, on both the side of AppSync and the dev server, which I assume were already working before I touched anything. I had gotten fragments and aliases working but I only really tested it on the dev server (which worked, although it felt super hacky), before realizing that it did not work with AppSync out of the box due to an oversight of mine, as it does not provide a way to get the full query with fragments alongside it. Not to mention that I ended up duplicating code between the dev server and the client.

However, AppSync does parse fragments already and returns the requested fields correctly in the selection set list it provides. So I intend to keep your code I had previously removed which used the selection set list, just having a second pass over the selection set GraphQL after the fact to add nested filters, while cherry picking some of the code I've already written. I'll essentially be testing against a request I want to get working on both the dev server and AppSync, which will attempt to use fragments (InlineFragment and FragmentSpread), aliases, nested queries, __typename, seeing if anything needs fixing and trying to take a minimalist approach to it to avoid potentially breaking other things.

Hopefully this makes sense? I am just trying to make Prisma AppSync more fun to use with developer-friendly GraphQL features and functional with GraphQL clients that require those features.

There is something I'm wondering though, do you know how GraphQL is supposed to behave when a field is requested twice in the same selection set?

maoosi commented 1 year ago

Yeah, that makes sense. Just so you are across - @tomschut is also working on a PR to replace the current local dev server with amplify-appsync-simulator (see https://github.com/maoosi/prisma-appsync/discussions/130). This will probably solve any discrepancies between the local server and AWS AppSync, add support for Fragments, and help reduce the overhead of maintaining the local dev server.

There is something I'm wondering though, do you know how GraphQL is supposed to behave when a field is requested twice in the same selection set?

I'm not 100% sure there is an established rule on this, but Yoga GraphQL seems to ignore duplicated fields (act as if there was only requested once).

Tenrys commented 1 year ago

Got it, I'll just have to test what the current behavior is then. I don't see any PR aside from that discussion yet so I think I'll keep working with the Yoga dev server for now, although I will take a look at that package as well.

Tenrys commented 1 year ago

So... I tried taking a look at what I could do, and it doesn't look like aliases or nested field arguments can reliably be parsed, because AppSync does not give you enough information.

  1. Aliases
    • The aliased field is passed as-is in the selectionSetList, which makes Prisma query the wrong field. There is no information as to what field that alias is actually trying to fetch. Sure, you could parse selectionSetGraphQL, but you would be missing information regardless if your selection set happens to include a fragment defined outside of it, as that information is also missing from the AppSync event payload data.
  2. Nested field arguments
    • Parsing selectionSetGraphQL would do it, but it's the same issue as the above, fragments defined outside of it would not be able to resolve properly either.

At the very least, it does seem like the Amplify dev server works wonders. Unless there is somehow another solution that I couldn't find while googling or searching through the AWS AppSync Commnuity GitHub repository's issues, all we can do is avoid aliases and/or split up queries for nested filtering on lists.

tomschut commented 1 year ago

I don't know if it's still current, but I think I'm able to create a pr for amplify in the near future. We're already using it on our end but I haven't gotten around to creating the pr.

Tenrys commented 1 year ago

I think it would be worthwhile yeah!