Multiple field and access

[Vincz]

Q	A
Bug report?	yes
Feature request?	no
BC Break report?	yes
Version/Branch	0.13.2

When using annotations, if we found a multiple relation with doctrine (OneToMany or ManyToMany), we generate a non null multiple (like: [Friends]!). It means that the field will always be an array (but it can be empty). So we have a Type::nonNull field. If we have an access on this field, like @=isGranted() and we query for this field without being allowed, it will return a null value for the entire object instead of just for this fields itself (I haven't been able to really trace the problem, but I think it is in webonyx/graphql).

There is multiple ways to solve this problem.

1. Generate nullable type instead of non nullable type
2. Detect if we have an "access" on a field and if the field is "multiple", remove the non nullable
3. Try to detect the case in the request handling and return an empty array instead of a null value

Personally, I would prefer the second one as if we have a field with an access, we are supposed to be able to return null for this field.

What do you think about this guys ? ping @mcg-web @murtukov @akomm ?

I make a PR.

[murtukov]

Hi, @Vincz.

Why are you trying to fix the side effect caused by another bug? If I understand correctly, the access option should not return null for the entire object and this is the bug we should fix. The 3 options that you suggest look like a temporary hack.

[Vincz]

In fact, I'm not even sure it is a bug. I checked the code of webonyx on fields resolution, and when a field is marked as not null, if the resolution failed (ie. An exception is throw) for this field, the entire operation failed and we get a null for the entire operation. I think that's because we are not supposed to have exception raised during resolution. See this line. Note that it seems to have change in the non published version of webonyx/graphql

The way the access stuff works in the bundle, is by adding a resolver that raise an exception if the access is denied, and it's in the webonyx lib that the exception is handled.

Maybe we should change the logic on our side, I don't know. By the way, what is exactly the "concept" behind our access feature. If I query a field I don't have access to, should I get an error or should I get an empty value for this field. Or should it be an option?

[mcg-web]

Hi @Vincz, yes this is not a bug if your client is not enough "smart" to ask only fields that viewer has access then you should make these fields nullable. You could also help your client to know the fields it have access before making the request:

{ 
 viewer {
   access {
     blog { fields }
  }
}

{
  "viewer": {
    "access": { "blog": { "fields": ["id", "name"] } }
  }
}

[Vincz]

Hey @mcg-web ! My use case is a little bit different. The "accessibility" of the field depends on the data of the object itself.

Let me explain: Imagine I have an object Profile with two fields : email and isEmailPublic. If I query a Profile, I should get a value for email, only if the Profile isEmailPublic, otherwise, I should get a null for the email. So I don't really want to make two queries, one to get the isEmailPublic field, and then a second one to get the email field (and I don't want to deport the access logic on the client). So my client ask for all the potential data and builds the UI depending on the result. And I don't want to have any error either.

Maybe we could add an option on the access feature (something like 'returnEmptyOnDenied') to specify that in case of denial on the field, we should return an empty value and no error ? It would then be handled in the AccessResolver. It would give more flexibility to the access feature, as it would allow complex use cases server side. What do you think?

[mcg-web]

Yes this new returnEmptyOnDenied option could be a more flexible solution.

[Vincz]

Yes this new returnEmptyOnDenied option could be a more flexible solution.

👍❤️ I’ll work on it !

[akomm]

@Vincz In general I'm with this comment. It depends on whether you really need the isEmailPublic. I am judging only by the info I have so far:

Of all proposed & preferred solutions here, all seem to change the field to nullable. What is the reason, not to make it nullable in the schema? Because it tells, what the field really can become. It can be null, if you have no access to it. Then you would not need isEmailPublic field at all. Except you want to tell the viewer why no E-Mail is visible - e. G. because no E-Mail defined or because not public.

Obviously, the client would have to check value != null, instead of isEmailPublic to see, if email(s) are public. But its known to client by schema. But thats just my thought on it, judging by the info given so far.

[Vincz]

Yes this new returnEmptyOnDenied option could be a more flexible solution.

@mcg-web Do you think I should add the option on the side or redefine the way the access option is defined ?

Something more like this :

MyType:
    fields:
        field1:
            access: "@=isAuthenticated()"
            accessEmptyOnDenied: true

...or...

MyType:
    fields:
        field1:
            access: 
                handler: "@=isAuthenticated()"
                emptyOnDenied: true

In the second case, we could also accept the "old" format to be bc compatible too.

[Vincz]

ping @mcg-web

[mcg-web]

Hey @Vincz sorry for late reply I missed this one. The option emptyOnDenied could still be helpful I think but It can be difficult to implement.

[Vincz]

Hey @mcg-web ! I'm currently working on this. So I ended up adding a new field config key accessConfig and a new type config key defaultFieldsAccessConfig. As they are separate, it's easy to mix them to cover more use cases (like setting globally the config for the whole type with defaultFieldsAccessConfig but having a custom access on a field without having to redefine the accessConfig.

So the new config is an array, with currently only one boolean node nullOnDenied. Have a question regarding the config. It looks like that :

->arrayNode('accessConfig')
     ->addDefaultsIfNotSet()
     ->children()
          ->booleanNode('nullOnDenied')->defaultFalse()->end()
     ->end()
->end()

Should I use the addDefaultsIfNotSet and have all generated types config with an accessConfig key defined but keep the 'default' logic only in the Configuration (but the builded type will only have the accessConfig if the access is used).

Or should I not use the addDefaultsIfNotSet so there is no accessConfig key when it is not used, but I would have to duplicate the 'default' logic in the AclConfigProcessor and add something like that to handle the 'default value':

$nullOnDenied = $field['accessConfig']['nullOnDenied'] ?? false;

For me the first option is cleaner as the default is handle only in the configuration and at the end it doesn't impact performances as the final config array is just use to generate compile the type classes.

What do you think?

[mcg-web]

@Vincz both are good the first solution is good if we use this only once (single method or class), the second is better for reusable mater.