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.

Sign up for GitHub

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

Jump to bottom

Add real path to ResolveInfo #1345

Open
ruudk opened this issue Mar 9, 2023 · 6 comments · May be fixed by #1548
Open

Add real path to ResolveInfo #1345

ruudk opened this issue Mar 9, 2023 · 6 comments · May be fixed by #1548
Labels
enhancement

Comments

@ruudk
Copy link
Contributor

ruudk commented Mar 9, 2023

I'd like to be able to get the real (trusted) path in ResolveInfo next to the untrusted aliased path.

Similar to what was written in #412.

query {
  myAlias: actor {
    myOtherAlias: movies
  }
}

ResolveInfo->path will contain myAlias.myOtherAlias instead of actor.movies.

Why is this useful? It can be used to quickly log the path of a requested field. For example, when somebody does not have access. By logging the real path, you never have to guess what it was. Also, it allows trusting the path because it cannot be spoofed.

What would be the best way to implement this? I tried adding an additional safePath to ResolveInfo, but that requires many changes in ReferenceExecutor.

Another way could be to introduce a PathAlias value object that holds the alias + field name. For example:

protected function executeFields(ObjectType $parentType, $rootValue, array $path, \ArrayObject $fields)
{
    // ...
    foreach ($fields as $responseName => $fieldNodes) {
            $fieldPath = $path;
            if ($responseName !== $fieldNodes[0]->name->value) {
                $fieldPath[] = new PathAlias($fieldNodes[0]->name->value,  $responseName);
            } else {
                $fieldPath[] = $responseName;
            }

So ResolveInfo->path could become [PathAlias('myAlias', 'actor'), PathAlias('myOtherAlias', 'movie'), 'some', 'other', 'field'].

We could put this behind a feature flag to not break existing projects that rely on the ResolveInfo->path.

Another simpler way could be to do this:

protected function executeFields(ObjectType $parentType, $rootValue, array $path, \ArrayObject $fields)
{
    // ...
    foreach ($fields as $responseName => $fieldNodes) {
            $fieldPath = $path;
            if ($responseName !== $fieldNodes[0]->name->value) {
                $fieldPath[] = sprintf('%s: %s', $responseName, $fieldNodes[0]->name->value);
            } else {
                $fieldPath[] = $responseName;
            }

This would produce ['myAlias: actor', 'myOtherAlias: movies'].

What do you think? I'd be happy to work on a PR to get this improved.
@spawnia
Copy link
Collaborator

spawnia commented Mar 10, 2023

Consider that without aliases, the "real path" can be ambiguous or have conflicts.

{
  foo: bar {
    qux
  }
  baz: bar(arg: 3) {
    qwak
  }
}

I would not change existing functionality, but could see the addition of another API that allows retrieving the unaliased path.

@ruudk
Copy link
Contributor Author

ruudk commented Mar 10, 2023

I would not change existing functionality, but could see the addition of another API that allows retrieving the unaliased path.

So next to a path we keep a safePath or realPath? Would that work for you? If so, then I'll provide a PR.

@spawnia
Copy link
Collaborator

spawnia commented Mar 10, 2023 •
edited

If possible, I would like to see it implemented as a method in order to calculate it lazily. And I would call it unaliasedPath, that explains what it is.

@ruudk
Copy link
Contributor Author

ruudk commented May 26, 2023

To calculate it lazily, what's the best approach?

I was thinking to add a method to ResolveInfo that uses the path and then walks through the operation selection sets again to find the names by alias.

It feels nasty to me. Are there plans for v16 where we can break this behavior? And make it so that path is not just a list of strings, but a list of Path objects that contain the name and the nullable alias?

@spawnia
Copy link
Collaborator

spawnia commented May 28, 2023

If lazy calculation is tricky and eager addition is easy and cheap, we can also add it as a property, I am not totally against that. My guess is that having two separate, simple lists is better. I don't see a scenario where one would care about aliases for some parts of the path - either you want aliases or you do not. A list of strings is easier to work with.

ruudk added a commit to ruudk/graphql-php that referenced this issue Mar 24, 2024
@ruudk
Add unaliased path to resolve info
6808e9f 
Fixes webonyx#1345
@ruudk ruudk linked a pull request Mar 24, 2024 that will close this issue
Open
ruudk added a commit to ruudk/graphql-php that referenced this issue Mar 24, 2024
@ruudk
Add unaliased path to resolve info
4331fde 
Fixes webonyx#1345
@ruudk
Copy link
Contributor Author

ruudk commented Mar 24, 2024

Implemented in #1548

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add unaliased path to resolve info ruudk/graphql-php
2 participants
@ruudk @spawnia