renamedFilterFields not working

adamgroom commented 3 months ago

I have upgraded to version 3.3.1, previously I was using filterFields, I have updated this to renamedFilterFields, though the filtering isn't working:

   return $this->sortFields([
            'persons.first_name' => 'name',
            'persons.last_name' => 'surname',
            'patients.date_of_birth' => 'dob',
            'patients.nhs_number' => 'nhsNumber',
            'project_patients.referral_date' => 'referralDate',
        ])->sort()
        ->renamedFilterFields([
            'persons.first_name' => 'name',
            'persons.last_name' => 'surname',
            'patients.date_of_birth' => 'dob',
            'patients.nhs_number' => 'nhsNumber',
            'project_patients.organization_patient_code' => 'projectCode',
            'projects.client_id' => 'clientId',
            'contacts.email' => 'contactEmail',
            'contacts.name' => 'contactName',
            'patients.is_vulnerable' => 'vulnerable',
            'patients.is_high_risk' => 'highRisk',
            'project_patients.project_id' => 'projectId',
            'project_patients.referral_date' => 'referralDate',
            'pathway_patients.project_pathway_id' => 'pathwayId',
            'pathway_patients.status' => 'pathwayStatus',
            'patient_flags.patient_flag_type_id' => 'patientFlagTypes',
        ])
        ->filter()

adamgroom commented 3 months ago

I managed to get it working by populating filterFields with the names that I was renaming fields to:

->filterFields([ 'referralDate' ])

abbasudo commented 3 months ago

@adamgroom hello. Thanks for reporting. Surely that's, not expected behavior. We will fix that.

adamgroom commented 3 months ago

Hi @abbasudo

Thank you, it doesn't get past $this->validateField($field); in Resolve.php as it's not in the available fields Array

 private function applyRelationFilter(Builder $query, string $field, array $filters): void
    {
        foreach ($filters as $subField => $subFilter) {
            $relation = end($this->fields);
            if ($relation !== false) {
                array_push($this->previousModels, $this->model);
                $this->model = $this->model->$relation()->getRelated();
            }
            $this->validateField($field);
            $this->validateOperator($field, $subField);

            $this->fields[] = $this->model->getField($field);
            $this->filter($query, $subField, $subFilter);
        }
        array_pop($this->fields);
        if (count($this->previousModels)) {
            $this->model = end($this->previousModels);
            array_pop($this->previousModels);
        }
    }

    /**
     * @param string $field
     *
     * @return void
     */
    private function validateField(string $field): void
    {
        $availableFields = $this->model->availableFields();

        if (!in_array($field, $availableFields)) {
            throw FieldNotSupported::create($field, $this->model::class, $availableFields);
        }
    }

adamgroom commented 3 months ago

This fixes it

` /**

@return array */ public function availableFields(): array { if (!isset($this->filterFields) && !isset($this->renamedFilterFields)) { return array_merge($this->getTableColumns(), $this->relations()); }
```
return $this->getUserDefinedFilterFields();
```
}

/**
Get formatted fields from filterFields.
@return array */ public function getUserDefinedFilterFields(): array { if (isset($this->renamedFilterFields)) { return $this->renamedFilterFields; }
```
if (isset($this->userDefinedFilterFields)) {
    return $this->userDefinedFilterFields;
}`
```

abbasudo commented 3 months ago

@adamgroom thanks for reporting and fixing this issue. I will draft a new version.

abbasudo commented 3 months ago

new fixed version

Lakshan-Madushanka commented 3 months ago

@abbasudo, @adamgroom, There seems to be a misunderstanding regarding the renamedFilterFields feature. It is not intended as a substitute for availableFields, as its name might suggest. Its purpose is solely to define if your query string field differs from the database column.

In this case you have to do like below,

->availableFields([
            'persons.first_name'
            'persons.last_name'
            'patients.date_of_birth'
           ...
)]
->renamedFilterFields([
            'persons.first_name' => 'name',
            'persons.last_name' => 'surname',
            'patients.date_of_birth' => 'dob',
           ...
)]

It is obviously repetitive, but it's solid and clear. Merging #61 may confuse the features and their behaviors. For instance, if you need to restrict a filter for a column, you have to explicitly define availableFields.

I think to avoid this repetition, we should allow renaming fields in the availableFields array as below.

$availableFields = ['persons.first_name,name' => '$eq',]

adamgroom commented 3 months ago

Hi Lakshan

It wasn't clear to me how to use renamedFilterFields from the documentation, I had to look at the source code to figure out what I considered a workaround.

I'm not too fussy about the implementation, though I think it should be clear in the docs and we should avoid breaking previous versions.

If you do intend to revert the commit, could you please give me some warning so I can update my code.

I'm happy to help with the new feature and also improve the documentation.

Kind regards Adam

On Wed, 15 May 2024 at 14:06, Lakshan Madushanka @.***> wrote:

@abbasudo https://github.com/abbasudo, @adamgroom https://github.com/adamgroom, There seems to be a misunderstanding regarding the renamedFilterFields feature. It is not intended as a substitute for availableFields, as its name might suggest. Its purpose is solely to define if your query string field differs from the database column.

In this case you have to do like below,

->availableFields([ 'persons.first_name' 'persons.last_name' 'patients.date_of_birth' ... )] ->renamedFilterFields([ 'persons.first_name' => 'name', 'persons.last_name' => 'surname', 'patients.date_of_birth' => 'dob', ... )]

It is obviously repetitive, but it's solid and clear. Merging #61 https://github.com/abbasudo/laravel-purity/pull/61 may confuse the features and their behaviors. For instance, if you need to restrict a filter for a column, you have to explicitly define availableFields.

I think to avoid this repetition, we should allow renaming fields in the availableFields array.

— Reply to this email directly, view it on GitHub https://github.com/abbasudo/laravel-purity/issues/60#issuecomment-2112467958, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQRUFGKP7LJNWAK6NEFHPTZCNMVZAVCNFSM6AAAAABHQXXXXOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJSGQ3DOOJVHA . You are receiving this because you were mentioned.Message ID: @.***>

Lakshan-Madushanka commented 3 months ago

@adamgroom Sorry for the inconvenience. The idea is that if you need to restrict your filter fields, you must define all of them in the availableFilters array. So, any other fields outside availableFilters are considered invalid.

If you need to use different names for DB columns in query string field you must use $renamedFilters array. You only need to define renamed columns there.

For example, if you only need to allow filters for the "name" and "age" columns, and the "name" column is expected as "user_name".

$availableFields = ['name', 'age'];
$renamedFields = ['user_name' => 'name'];

After digging into the source code, I discovered that the expected behavior mentioned above doesn't work due to a bug here.

Thanks you for pointing these issues. You may try to fix it #63 otherwise we'll fix them asap. After that we'll move to new feature.

adamgroom commented 3 months ago

I see, so my fix created a new bug as using renamedFilterFields would prevent filterFields from being used?

On Wed, 15 May 2024 at 15:13, Lakshan Madushanka @.***> wrote:

@adamgroom https://github.com/adamgroom Sorry for the inconvenience. The idea is that if you need to restrict your filter fields, you must define all of them in the availableFilters array. So, any other fields outside availableFilters are considered invalid.

If you need to use different names for DB columns in query string field you must use $renamedFilters array. You only need to define renamed columns there.

For example, if you only need to allow filters for the "name" and "age" columns, and the "name" column is expected as "user_name".

$availableFields = ['name', 'age']; $renamedFields = ['user_name' => 'name'];

After digging into the source code, I discovered that the expected behavior mentioned above doesn't work due to a bug here https://github.com/abbasudo/laravel-purity/blob/5dc70b98c6548e56a175c9702566e86962354b51/src/Traits/Filterable.php#L141C26-L141C45 .

Thanks you for pointing these issues. You may try to fix it otherwise we'll fix them asap. After that we'll move to new feature.

— Reply to this email directly, view it on GitHub https://github.com/abbasudo/laravel-purity/issues/60#issuecomment-2112665240, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQRUFDMYX33VEIFXSENSZDZCNUP7AVCNFSM6AAAAABHQXXXXOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJSGY3DKMRUGA . You are receiving this because you were mentioned.Message ID: @.***>

Lakshan-Madushanka commented 3 months ago

@adamgroom It wasn't your fix. Bug was there from the beginning.

abbasudo commented 3 months ago

@Lakshan-Madushanka Hi.

You are right. This may confuse developers. However if someone added a field to renamedFilterFields() in the controller he certainly want to allow using it. I suggest we apply this feature only for renamedFilterFields() function and not for $renamedFilterFields variable in the model. this means adding a field to $renamedFilterFields in model will not allow it. but adding it through renamedFilterFields() will allow using it. Thoughts?

Lakshan-Madushanka commented 3 months ago

@abbasudo, It's a bit awkward for me to have conflicting purposes for a single feature. I think the root of this issue lies in $filterFields needing all the fields available, but most of the time we only need the table fields + some renamed fields. Can't we do something like below instead?

$filterFields = ['name as title'];
$filterFieldsMode = 'addition';

This mode gives table columns + renamed fields as allowed fields.

I believe this will resolve many issues. In fact, we can define all configurations using only $filterFields.

abbasudo commented 3 months ago

@Lakshan-Madushanka adding mode is complex to understand and apply. let's stick to your first suggestion as mentioned in #65 .

abbasudo / laravel-purity

renamedFilterFields not working #60