Discussion: replace-field transform

[ntziolis]

@santino As suggested moving this into separate issue in #2862

We have been successfully using the transform. Here are some comments:

• the transform is very powerful and enables A LOT of things and we are already heavily making use of it
• it took us quite some time to understand how configuration done.
  • I think the documentation would benefit from a set of simple concrete use cases that don't build on each other
  • Which is not to say to not have one full blown example that pulls everything together as well
  • I'd be open to create a PR that builds on the existing documentation to assist others moving forward
• it feels like from / to are backwards. Any chance this could be reversed? This actually cost us the majority of time.
• one very powerful feature is the inline type + composer that can be used for intermediary purposes
  • it would be worth pointing out this use case more explicitly (that the type is not actually used in the end)
  • apart from that it would be great to specify type defs as part of a replacement in addition to globally all replacements
    • the list of replacements can get quite large
    • this way the
  • that said I also see use cases where a type def should span all replacements
• it should be possible to replace to fields that do not exist yet right now one has to go the route via an intermediary type
• retaining args when hoisting is often key use case when hoisting
  • any list call that supports pagination
  • but returns some funky / ugly return type that one just wants to clean up
  • without retaining the args that appear along the path this transform cant be used for this purpose

[santino]

Thanks for your feedback @ntziolis.

I understand it takes some time to learn the transform, because is very powerful and you can achieve a lot, the configuration has also several options. BTW, in case you didn't know, I added yesterday a feature to also rename fields as part of #2955; this has been released already in v0.1.0 and is documented, ~however, the website doesn't seem to be updated yet, so you can check the docs in the PR if you wish to use this.~ EDIT: the website has just been deployed, you can check documentation here (check your cache, since this is fresh).

• Regarding documentation. I think the documentation is already quite comprehensive and surely needs more attention compared to that of other transforms. So if you think there are some confusing parts, surely it can be reworded a bit. Also, I always suggest looking at the tests which are more comprehensive, compared to documentation, and offer real usage examples. If instead, you're looking for more concrete examples, maybe these can be added as actual code sandboxes so that people can interact with them. Please feel free to work on some examples and create a PR, I will be happy to assist with that.

• it feels like from / to are backwards You wouldn't believe that, but one of my colleagues also said the same! Whilst I now appreciate this might be more of a shared feeling, I would insist to keep it like it is currently. The usage of from and to is the same as other transforms like Rename. The idea is that from always represents the current schema, and to refers to what you want it to become. I mean, I will not impose to keep things like this, but it does seem both logical and consistent to me; maybe having an extra opinion from @ardatan will be useful on this point.

• type + composer The idea for this transform is for typeDefs and composer to be used in conjunction with type changing as well. If you just need type and composer, without replacing fields nor their return value;w you can already achieve this with resolvers-composition transform and additionalTypeDefs

• specify type defs as part of a replacement I thought about that and maybe it makes sense to do that with inline GraphQL definitions, but maybe not so much if you want to load additional .graphql files. I think I would be more inclined to allow passing an array of .graphql files in typeDefs so that you can keep your definitions more modular for different sources. One additional thing to keep into consideration is that this transform has been conceived for bare mode and ideally you use bare transforms on a single data source level, and not across all data sources. In this case, to me, ​it makes sense to have one .graphql file per data source. Type definitions are not about spanning across all replacements, you can think of these like an additional GraphQL schema that is merged with your original one, so all types are available (original and additional) and then the transform can pick from this new combined schema.

• replace to fields that do not exist yet right now Also thought of that, and maybe it can be added. My initial thought is that this sort of manipulation is best done separately as a schema extension. The intent of this transform is to override parts of a schema, but if you need to introduce new parts, schema extension is probably the way to go and is already supported by mesh, as mentioned in the links shared above.

• retaining args This definitely makes sense. You can already use scope: config to retain pretty much everything from the original field, including args and resolve function. That can probably be too much though, so I could simply add a new scope for args or maybe combine multiple scopes as an array. Obviously right now you can still achieve args on a hoisted field using typeDefs so that you define a new Type that has the args you need, but this is more of a manual work of course, and we can simplify it.

[ntziolis]

Thanks for taking the time to write such a detailed response. I will weave in my responses below.

After playing more with the transform and taking into consideration the new functionality:

• I believe the transforms functionality is an absolute must, its super helpful and I wouldn't wane miss it moving forward
• At the same time I believe the transform is also missing some small functionality to round it off + could benefit from a more descriptive configuration

Please bear with me as I understand the below would change the nature of the transport significantly.

What I think the transform should be able todo:

• The transform should copy a field from a source type to a field on the target type
• The transform should not care if the target type already has a field with the name in question
  • This allows to add fields to types (without replacing existing fields)
  • This would mean there is no need for a use to specify renames explicitly
• The transform should be able to optionally
  • remove the source field from the source type
  • remove any field on the target type (to enable replace + rename use case)
• In addition I would keep the typedef + compose stuff as is (I omitted this from the config below as it should stay the same)
  • It can be used as fallback, but I believe the need for using this will go down dramatically
• (At a later stage) Allow use of regex similar to rename

Based on the above lets reimagine how the import configuration could be structured

transforms:
  - copy-fields: # Not married to the name, but replace seems incorrect now since its only one of the use cases
      steps:
        - source:
            field: Book.code # this notation opens up enabling regex later on
            removeAfterCopy: true # default should be false if the name of the transform is copy
          target: 
            field: NewBook.isAvailable # if field exists it gets overridden when it does not exists it is added automatically
            removeAfterCopy: anyFieldNameOnSameTypeAsTargetField # this is to emulate replace + rename
          scope: hoistValue

What do we gain from this:

• Simplified configuration for common use cases
• Additional use cases while not increasing complexity of configuration
• More clarity on to the from / to question (even though this should not be a deciding factor)
  • Now to does represent "what you want it to become" as the field name configured might not even exist on the type yet,
  • Hence it cannot represent the current schema

Side note on why regex patterns might be interesting:

• We often encounter REST APIs that have a uniform structure (for example across their search endpoints)
• However the structure doesn't really fit into the graphql world (would result in unnecessary nesting etc.)
• Today we have at least 5+ config lines per endpoint we wane change, usually we have 30-50 endpoints where this needs to be done. Which this leads to unnecessary large configurations
• With regex patterns we would be able to create one single step that would straighten out all these in one go

I wanted to get your feel for what you think about the above prior to diving into each of the points you mentioned above.

[santino]

Thanks, @ntziolis.

Reading your message makes me think that perhaps what you need is a different transform. Replace-field is there to override parts of the schema, where especially you want to affect the behavior of those parts. Hoisting values and applying extra custom logic (through composers) are some of the examples of how you change the behavior of the schema bits you replace.

Having a single transform that does it all might probably become too much as it even gets more complex. I think having several transforms in place that can even act on the same schema parts is more modular and easy to configure/understand.

If I understand, what you probably need is a "move-field" transform that is able to move and/or copy/duplicate fields from one Type to one or more Types across your schema. That transform could have scopes like "move" or "copy" that define if a field is also removed from the original type or just copied/duplicated onto another type.

Once you move all your fields in the way you want you can then apply replace-field to override the behavior of a field. If you just need to do value hoisting, maybe a HoistField transform can also be introduced that does just that.

I wouldn't mess with things like "remove any field on the target type". I would still leave that to the good old filter-schema transform.

We can start thinking about the new transforms as I am stating above. Maybe once we have some of these, the use for replace-field will seem less fundamental. If you have one transform to move/copy fields, one to hoist values, and the existing transform to apply resolvers composition; probably replace-field might become redundant.

[ntziolis]

@santino I think you are spot on with this.

We can start thinking about the new transforms as I am stating above. Maybe once we have some of these, the use for replace-field will seem less fundamental. If you have one transform to move/copy fields, one to hoist values, and the existing transform to apply resolvers composition; probably replace-field might become redundant.

I agree that having smaller less complex transforms would be preferred. Until then lets keep the replace field as is. I will have a look at creating the move/copy transform heavily borrowing from replace-field. I think the hoist transform can stay as is. That said I would wait to PR both these transforms together in sync with you.

I'd love some input on the copy / move transform:

• One option would be to have a transform that only copies which can be combined with filter transform to remove the field
• That said I feel like move is a regularly appearing case and it would be nice to not have to employ multiple transforms
• This leaves two options one transform for each or one transform that handles both copy + move
• The biggest problem would be what to name the copy+move transform,
  • if we name it copy it might not be clear you can also remove the field
  • and if we name it move it might not be clear you can also just copy
  • I'm sry of this seems like a dumb point, maybe I have stared at this for too long xD

Let me know which route you prefer in regards to move + copy and I will start the work on those.

[santino]

From my response above:

... a "move-field" transform that is able to move and/or copy/duplicate fields from one Type to one or more Types across your schema. That transform could have scopes like "move" or "copy" that define if a field is also removed from the original type or just copied/duplicated onto another Type/s.

So from there, I see a single transform that takes scopes to perform move/copy actions.

The naming is a real point, not dumb at all. Maybe we can call this transform transfer-field, with scopes copy and move.

To start with it can have an API like this:

transforms:
  - transfer-field:
      transfers:
        - from: Query.availableBooks
          to: Author.books
          scope: copy

Note how the field name is changing from "availableBooks" (source) to "books" (target). Also, note to use Type.FieldName syntax for the config, to avoid potential issues with nested fields. The above to/target example would refer to this schema tree: Query.author.books The above is just an example, clearly for a transfer like that you'd need to implement an additional behavior on the copied field to filter books by author, for example; which can already be achieved today with resolver composer.

Probably scope: copy could be the default value, when scope is not passed (undefined), copy is applied; whilst "move" must be passed explicitly when you want to get rid of a field from source.

From/to can be named source/target potentially, I am not passionate about it; I'd feel like from/to should be fine and in line with Rename transform.

RegEx would be useful, but it doesn't necessarily need to be part of the initial layout. If easier you might just start with a PR that implements basic functionality of the transform and we take it from there. If you do want to experiment with RegEx, maybe an idea to keep code simpler is to introduce a config like useRegExp: true, again similar to what it's there in Rename transform.

As a Mesh transform, this would be really useful when developers need to move fields that are automatically placed in Mutation/Query, since not all handlers are able to do this correctly. This is where the "move" scope will surely shine.

transforms:
  - transfer-field:
      transfers:
        - from: Mutation.authors
          to: Query.authors
          scope: move
        - from: Query.availableBooks
          to: Author.books
          scope: copy

Finally, it might be nice to copy a field to multiple types, so to could take an array like this: to: [Author.books, Catalogue.books]

Let me know your thoughts.

It's great that you're willing to work on this transform but keep in mind that I'm also happy to help with this.

[Lappihuan]

This looks exactly like what i would need too. With the SOAP source every action is mapped to mutation Type which is just wrong but i get why its happending. I would need to be able to move Mutations to Queries.

Ideally lets not forget the very powerfull regex options which can help reduce the size of the config.yaml like:

transforms:
  - transfer-field:
      transfers:
        - from: Mutation.(get.*)
          to: Query.$1
          useRegex: true
          scope: move

Has some work on this already started which i could contribute to?

[santino]

The work hasn't started yet. We've been discussing it here and @ntziolis has volunteered to work on it.

@ntziolis if you find it challenging to dedicate time to this right now, I will be happy to get started, and can probably aim to get a PR ready for mid next week.

[ntziolis]

@santino I was aiming for working on this on the weekend, but If you want to get started no issues from my end.

@lappihuan We are running into very similar issues with legacy / badly designed REST endpoints, We are already making heavy use of the regex ability of rename-transform. Any regex functionality would be similar to that. That said Regex is def secondary right now. It might not be included in the first versions of these transforms. But it won't be forgotten as we need it ourselves as well ;)

[ntziolis]

@Lappihuan I created an initial version of what I think the transfer-field transform should be able todo, see here #3041.

Some deliberate limitations

• Regex can't be applied to type name
  • As of right now I cant think of a use case
  • Please let me know if you feel this should be added
• In this regard please note that I plan to extend the hoist transform to support glob patterns in the path #2862
  • this is to enable use cases e.g. where all list calls, return responses where the actual results are wrapped in some intermediary type
  • assuming the structure of these intermediary results is the same + its possible to create a glob pattern that matches all list calls it would then be possible to hoist the actual results and remove the intermediary type for all list calls at once

Please let me know your thoughts.