Computed properties issue

[zjbarg]

Consider the following:

type User {
   id
   name
}
type Post {
   id
   title
   body
   isPublished
   publishedAt
}

Where isPublished is defined as a computed property as

class Post extends Model
{
    ...
    public function getIsPublishedAttribute(): bool
    {
        $publishedAt = $this->published_at;
        return $publishedAt !== null;
    }
}

where is_published published_at is a dateTime field in the database. Edited

So, isPublished is not selectable. The problem is the value of isPublished depends on published_at and the following query will always give isPublished : false

{
  users {
    id
    name
    posts {
      id
      isPublished #always false
    }
  }
}

and

{
  users {
    id
    name
    posts {
      id
      publishedAt
      isPublished #correct result
    }
  }
}

I suppose we need something like

            'isPublished' => [
                'type' => Type::nonNull(Type::boolean()),
                'selectable' => false,
                'must_select' => 'publishedAt'
            ],

because one wouldn't want to select published_at always, yet want it to always be selected when isPublished is selected

[mfn]

Philosophical note: I'm wondering if it's smart to add more and more custom attributes to such a config, mixed with the ones from GraphQL. I thinking about something funneling them in a single attribute, e.g.:

            'isPublished' => [
                'type' => Type::nonNull(Type::boolean()),
                'select_fields' => [
                    'selectable' => false,
                    'must_select' => 'publishedAt',
                ],  
            ],

Just thinking out loud and, sorry, unrelated to the issue itself: but I acknowledge a new directive is probably required.

Suggestions:

• naming: requires?
• it should probably be an array because who knows how many other columns it requires
• (and it should also support selecting relations because that's the next thing coming up)
• …

[mfn]

After one more thought, I'm not sure there is a problem, please see https://github.com/rebing/graphql-laravel/pull/378#issuecomment-508571375

[zjbarg]

I know always fixes the issue, but its a workaround,

because one wouldn't want to select published_at always, yet want it to always be selected when isPublished is selected

[zjbarg]

This is why i came across #369. I have a lot if computed properties in my project, Some of them depend on morph relations :P

[mfn]

You're right, acknowledged!

[zjbarg]

Suggestions:

• naming: requires?
• it should probably be an array because who knows how many other columns it requires
• (and it should also support selecting relations because that's the next thing coming up)
• …

Yes 'required' sound better. But I guess we can name it always because its doing basically the same thing. It appears to be a generalization on always. What do you think @mfn?

[mfn]

But won't this clash in some way? 🤔

I guess I need to judge this on a technical implementation rather 🤷‍♀️

[mfn]

because one wouldn't want to select published_at always, yet want it to always be selected when isPublished is selected

@ZaidBarghouthi I would like to revisit this, because every time I re-think about this it seems to me the 'always' is the correct technical solution. Or I'm missing something.

always in fact acts like the suggested requires: only on the fields being selected it's evaluated and thus adds the additional fields "always" to the SQL.

But it does not add the fields mentioned there always when the particular type is fetched.

Do you know what I mean?

When in \Rebing\GraphQL\Tests\Database\SelectFields\ComputedPropertiesTests\PostType I change this:

            'isPublished' => [
                'type' => Type::nonNull(Type::boolean()),
                'selectable' => false,
                'always' => [
                    'published_at',
                ],
            ],

Only when isPublished is actually requested in the query, is published_at also fetched. But it's not fetched when I don't specify isPublished.

[mfn]

ping @ZaidBarghouthi

[zjbarg]

Ah!, it seems to solve the problem!