renamedFilterFields not working

[adamgroom]

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]

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

->filterFields([
'referralDate'
])

[abbasudo]

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

[adamgroom]

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]

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]

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

[abbasudo]

new fixed version

[Lakshan-Madushanka]

@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]

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 <#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 <#60 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQRUFGKP7LJNWAK6NEFHPTZCNMVZAVCNFSM6AAAAABHQXXXXOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJSGQ3DOOJVHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[Lakshan-Madushanka]

@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]

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 <#60 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQRUFDMYX33VEIFXSENSZDZCNUP7AVCNFSM6AAAAABHQXXXXOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJSGY3DKMRUGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[Lakshan-Madushanka]

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

[abbasudo]

@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]

@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]

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