search

silverstripe / silverstripe-graphql

Serves Silverstripe data as GraphQL representations
BSD 3-Clause "New" or "Revised" License
52 stars 61 forks source link

Search Params/DataObjectQueryFilter Bugs #265

Open HARVS1789UK opened 4 years ago

HARVS1789UK commented 4 years ago

Affected Version: 3.2.3

Issues

Whilst following the documentation for exposing existing DataObjects through procedural code, I have come across the following issues/errors, which I intend to submit a PR to correct, this issue serves as some documentation of the issues and a place to discuss the accuracy of me reporting them as bugs (as I am happy to be corrected and shown how to implement the module correctly in it's current state).

All 3 issues relate to the below section of the docs (or something closely related to them) seen here - https://github.com/silverstripe/silverstripe-graphql/tree/3.2#adding-search-params-read-operations-only

A key snippet for examples to follow:

$scaffolder->type(MyDataObject::class)
  ->operation(SchemaScaffolder::READ)
    ->queryFilter()
      ->addDefaultFields('MyField')
      ->addFieldFilter('MyInt', 'gt')
      ->addFieldFilter('MyInt', 'gte')
    ->end()
  ->end();

1 - Documentation

The documentation refences the use of the following methods with regards to adding search parameters via the Filter GraphQL input:

addFilteredField('Title', 'contains') addDefaultFields('MyField')

Neither of these seem to exist and I think they are typos? The correct methods seems to be

addFieldFilterByIdentifier('Title,contains') addDefaultFilters(MyField)

The docs also show the use of addFieldFilter('MyInt', 'gt'), this method does exist, but expects something implementing FieldFilterInterface as the second parameter, not a string, so again in this context, I think the intended method is addFieldFilterByIdentifier('MyInt,gt')

see:

2 - DataObjectQueryFilter and the Chainable trait

DataObjectQueryFilter implements the Chainable trait and is shown in the docs to be chain-able against the SchemaScaffolder (see main snippet above). However, this gives me errors claiming ->end() cannot be called on null and I believe this to be due to Read::__construct() not calling ->setChainableParent() on the DataObjectQueryFilter when it creates it?

See:

3 - DataObjectQueryFilter separator (__) vs Relationship dot notation (.)

Seemingly everything GraphQL uses __ as a separator between fields, related objects and filters etc, including the documentation for adding filters through procedural PHP code e.g.:

$this->addFilteredField('Categories__Title', 'in')

(see - https://github.com/silverstripe/silverstripe-graphql/tree/3.2#adding-search-params)

It is this __ separated way that the filtered are stored as key:value pairs in DataObjectQueryFilter::$filteredFields however, as we all know, SilverStripe DataList/DataQuery uses dot notation to detail fields on related objects e.g. Categories.Title

Between DataObjectQueryFilter::getFieldFilters() and DataObjectQueryFilter::getFiltersForField() there is a mix up between these formats and filtered fields referencing fields on related objects failed to get found (it is looking for DataObjectQueryFilter::$filteredFields['Categories.Title'] whereas it is stored as DataObjectQueryFilter::$filteredFields['Categories__Title'].

I have a crude fix to identify this and replace the . with __ before looking for the filter in that array, which does seem to fix it in my context, but I am not sure on the reliability of this in terms of other use cases/contexts which use this method?

My example fix:

    public function getFiltersForField($fieldName)
    {       
        if (strpos($fieldName, '.') !== false) {
            $fieldName = preg_replace("/\./", "__", $fieldName);
        }

        if (isset($this->filteredFields[$fieldName])) {
            return $this->filteredFields[$fieldName];
        }

        throw new InvalidArgumentException(sprintf(
            'Field %s not found',
            $fieldName
        ));
    }

Next Steps

I am happy to discuss these issues and willing to submit a PR to address some of them if suitable, but I need some support and input from the maintainers before doing so, namely:

HARVS1789UK commented 4 years ago

I have started a branch containing some updates relating to this issue in my own forked repo here - https://github.com/HARVS1789UK/silverstripe-graphql/tree/issue-265

ScopeyNZ commented 4 years ago

These (last 2) are bugfixes, so you can target a patch branch (x.x) that makes sense - 3.3 is the latest.

Docs isn't so important, but the latest minor is usually the default branch. It should get all merged up upon request (and hopefully if you've got docs updates bundled into a commit). I'll leave @unclecheese to comment on the nature of the fixes.

unclecheese commented 4 years ago

Thanks for submitting this. I'll have a look through today.

I guess the first thing that stands out to me is that this should reflect a leak in our test coverage, and when we shipped this feature, it shouldn't have passed PR without adequate tests. We should consider unit tests the source of truth for the documentation and the documentation itself just a human readable permutation of those docs. :)

So either the tests aren't right, or the docs aren't appropriately reflecting the tests, but either way I would start from what the unit tests are telling us.

HARVS1789UK commented 4 years ago

@unclecheese do you have any feedback on the code (rather than just docs) bugs referenced in points 2 & 3 or the suitability of my suggested fixes?

I am particularly keen to get No. 3 resolved as I am currently relying on my fix above being manually added in /vendor which is obviously going to get overwritten whenever I do a composer update etc

mleutenegger commented 4 years ago

I have stumbled upon 1) and 3) described in this issue while implementing a filter and have made a pull request (#267) for it.

@HARVS1789UK In order to completely cover this issue in said PR, I would like to include your changes fixing 2), if that would be ok with you or I can just give you access to the branch 😉.

@unclecheese any feedback concerning the PR would be welcome!

PS: @HARVS1789UK you can use the Injector pattern to monkey patch the problem in your application, allowing you to update dependencies using composer (at least thats what I'm doing right now). For this, simply extend DataObjectQueryFilter:

class DotDataObjectQueryFilter extends DataObjectQueryFilter
{
    /**
     * @param array $filters An array of Field__Filter => Value
     * @return \Generator
     */
    protected function getFieldFilters(array $filters)
    {
        foreach ($filters as $key => $val) {
            $pos = strrpos($key, self::SEPARATOR);
            // falsy is okay here because a leading __ is invalid.
            if (!$pos) {
                throw new InvalidArgumentException(sprintf(
                    'Invalid filter %s. Must be a composite string of field name, filter identifier, separated by %s',
                    $key,
                    self::SEPARATOR
                ));
            }
            $parts = explode(self::SEPARATOR, $key);
            $filterIdentifier = array_pop($parts);
            // If the field segment contained __, that implies relationship (dot notation)
            $field = implode('.', $parts);
            // The Field key is written with self::SEPARATOR
            $fieldName = implode(self::SEPARATOR, $parts);
            $filter = $this->getFieldFilterByIdentifier($fieldName, $filterIdentifier);
            if (!$filter instanceof FieldFilterInterface) {
                $filter = $this->getFilterRegistry()->getFilterByIdentifier($filterIdentifier);
            }
            if (!$filter) {
                throw new InvalidArgumentException(sprintf(
                    'Invalid filter "%s".',
                    $filterIdentifier
                ));
            }

            yield [$filter, $field, $val];
        }
    }
}

And then configure this class to be used instead:

SilverStripe\Core\Injector\Injector:
    SilverStripe\GraphQL\QueryFilter\DataObjectQueryFilter:
        class: DotDataObjectQueryFilter

The same should work for the SilverStripe\GraphQL\Scaffolding\Scaffolders\CRUD\Read class.