graphql-query-rewriter / core

Seamlessly turn breaking GraphQL changes into non-breaking changes
https://graphql-query-rewriter.github.io/core
MIT License
409 stars 16 forks source link

Add FieldNameRewriter #14

Open pmeskers opened 4 years ago

pmeskers commented 4 years ago

Hello there,

While using this rewriter, we came across a pretty simple case that seemed like it would be worth adding. Notably, we just want to rename a field somewhere in the query. Hopefully the tests help describe the use case.

It's pretty bare bones. Open to feedback, and if there is interest in going ahead with this I will update the README.

Many thanks!

pmeskers commented 4 years ago

I realize now this only handles the query rewriting, but not transforming the response back out. Will update.

edit: putting some thoughts on the path forward here. I might have to pause this effort for a bit. I believe the current way in which responses are rewritten does not support this use case: https://github.com/ef-eng/graphql-query-rewriter/blob/master/src/RewriteHandler.ts#L63-L80

Basically, for the node which had a positive match on the way in, we run the Rewriter's rewriteResponse function on the way out. However, we only invoke it with the resolved value at that path. So if we had the following:

// Rewriter: 
new FieldNameRewriter({
  fieldName: 'name',
  newFieldName: 'fullName'
})

// Query 
{
  person(id: "123") {
    name
  }
}

// matches { kind: 'Field', value: 'name' } in the AST

// Rewritten Query
{
  person(id: "123") {
    fullName
  }
}

// Response 
{ person: { fullName: 'John Smith' } }

the rewriteResponse function is only invoked with the value "John Smith", and does not offer any levers for re-shaping the data structure around it. I believe this has worked for the existing rewriters, whose values were always things serializing/deserializing as objects.

chanind commented 4 years ago

rewriteFieldName seems like a reasonable addition to the library! Originally something like that wasn't included because you can use the standard graphQL method of deprecating the old name, and adding a new field with the new name. That being said, I don't see any reason not to add this rewriter if it's something people want to do.

That's a good point about rewriteResponse. We could change the way rewriteResponse works so it gets passed in the object the current key is in as well as the key name, so it can have the chance to rewrite fields around itself like in this case. We'd just need to update the existing rewriters to work in that new paradigm, which shouldn't be too hard. There's only 1 or 2 rewriters that even do response rewriting currently.

Sytten commented 3 years ago

Can this be merged now that #20 is?