spatie / laravel-query-builder

Easily build Eloquent queries from API requests
https://spatie.be/docs/laravel-query-builder
MIT License
4.06k stars 397 forks source link

Call to undefined method #725

Closed punyflash closed 2 years ago

punyflash commented 2 years ago

Trying to migrate my project to Laravel 9. After upgrading laravel-query-builder to version 5.0.0 I get the error:

Call to undefined method Spatie\QueryBuilder\QueryBuilder::allowedAppends()

It seems like there is no more appending attributes point for version 5 as well. Why this feature was removed?

freekmurze commented 2 years ago

We felt like this feature didn't belong in the package and it added too much complexity.

pionl commented 2 years ago

Good to know, maybe update the "changelog" and release notes? Thank you

wi-wissen commented 2 years ago

For me, the Feauture appending attributes was very helpful. For example to add computed properties only when needed or allowed by role.

Fortunately you still kept all needed functions in QueryBuilderRequest. So I only had to restore AppendsAttributesToResults and InvalidAppendQuery and overwrite __call from QueryBuilder.

Is it planned to leave QueryBuilderRequest like this? If not, it would be a consideration changing variables like

private static $includesArrayValueDelimiter = ','; to protected for easier overwriting?

kurorido commented 2 years ago

I followed @wi-wissen suggestion and made a fork.

https://github.com/kurorido/laravel-query-builder/commit/067595efaeacfcb7c91f46df5f878ddce3cd9fb6

btxtiger commented 2 years ago

@freekmurze What is now the way to go to lazy add any computed data into the API response?? We were heavily using this feature

btxtiger commented 2 years ago

@kurorido how did you install the fork? Composer wouldn't allow me to install it, because the branch name "Invalid version string "main"

Update: Could solve it using

   "repositories": [
      {
         "type": "vcs",
         "url": "https://github.com/kurorido/laravel-query-builder.git"
      }
   ],

and "spatie/laravel-query-builder": "dev-restore_append"

euoia commented 2 years ago

I was also using this feature and was surprised by its removal after upgrading to Laravel 9.

@freekmurze Given that this is a breaking change, I would love to see some guidance on how to migrate existing code that was using this feature. Given that people were using this feature, UPGRADING.md could be worded a bit better:

The rest of the public API was not changed, so you'll be able to upgrade without making any changes.

I guess the simplest way is to modify your code to call append on the query result (or to always run the results through a resource).

In my own code, I have a class that wraps QueryBuilder (and adds pagination, aggregation, etc) so this was quite straightforward to do. I added validation like this:


    protected function validateAllowedAppends(Request $request)
    {
        // $allowedAppends was removed from Spatie's Laravel Query Builder in version 5.
        // https://github.com/spatie/laravel-query-builder/issues/725
        $appendStr = $request->input("append", null);
        $appends = $appendStr === null ? [] : explode(",", $appendStr);
        $appendsNotAllowed = [];

        foreach ($appends as $append) {
            if (collect($this->allowedAppends)->contains($append) === false) {
                $appendsNotAllowed[] = $append;
            }
        }

        if (count($appendsNotAllowed) > 0) {
            $allowedAppendsStr = implode(", ", $this->allowedAppends);
            $appendsNotAllowedStr = implode(", ", $appendsNotAllowed);

            throw new \Exception(
                "Requested append(s) `{$appendsNotAllowedStr}` are not allowed. Allowed append(s) are `{$allowedAppendsStr}`."
            );
        }

I do feel like there is a place for a library with these additional features (appending, pagination, aggregation, transformation, etc). @freekmurze do you know if that's something Spatie is likely to create? If not, I could possibly tidy up and release my version.

prodigy7 commented 2 years ago

That is a great pity that the functionality has been removed. This is important for me in some respects. Now I'll probably have to work with not so nice workarounds.

TheFrankman commented 1 year ago

Anyone come up with an elegant solution for this ? I was hoping to use this package as a means to resolve the append methods constantly causing query bloat. Maintaining a Fork can be quite troublesome and whose to say that future updates of this package will make the fork incompatible.

btxtiger commented 1 year ago

We recently found out that using the Model Attributes can create a huge performance lack, as it seems those functions get called for each model even if they are not explicitly required. So we decided to implement our own logic by using a custom queryParameter and naming the functions different than get....Attribute. This way we are also able to call nested functions if it is required. I can share our code later.

Edit:

The performance issues were caused by database columns that had the same name as the attribute functions. In this case, Laravel automatically updates the values by using the attribute functions, and automatically executes them.

TheFrankman commented 1 year ago

We recently found out that using the Model Attributes can create a huge performance lack, as it seems those functions get called for each model even if they are not explicitly required. So we decided to implement our own logic by using a custom queryParameter and naming the functions different than get....Attribute. This way we are also able to call nested functions if it is required. I can share our code later.

I would appreciate the code share. I like you have the same issue, it will be a massive task to unpick it at this stage, but at least we have unit tests !

harlan-zw commented 1 year ago

Can we at least keep this issue open (and renamed for clarity) to make it easier to find this issue until the breaking change is properly documented?

btxtiger commented 1 year ago

@TheFrankman This is how I solved it for now. I'll update this comment if I find some conflicts in my app. But it seems to work fine for now, with plain ->get() aswell as ->paginate().

if ($request->append) {
   $appends = collect(explode(',', $request->append));
   $data->each(function ($row) use ($appends) {
      $appends->each(function ($attr) use ($row) {
         $attrFnName = Str::camel('get_' . $attr . '_attribute');
         if (method_exists($row, $attrFnName)) {
            return $row->append($attr);
         }

         throw new \Exception("Append attribute <$attr> not found in model " . get_class($row));
      });

      return $row;
   });
}

Updated more complex API function:

protected function addAppendsFix(Request $request, Collection|LengthAwarePaginator|Model $data): Collection|LengthAwarePaginator|Model {
   if ($request->append) {
      $appends = collect(explode(',', $request->append));

      $callAppendFn = function ($row, $appends) {
         $appends->each(function ($attr) use ($row) {
            $attrFnName = Str::camel('get_' . $attr . '_attribute');
            if (method_exists($row, $attrFnName)) {
               return $row->append($attr);
            }

            throw new \Exception("Append attribute <$attr> not found in model " . get_class($row));
         });

         return $row;
      };

      if ($data instanceof \Illuminate\Pagination\LengthAwarePaginator || $data instanceof \Illuminate\Database\Eloquent\Collection) {
         $data->each(fn($row) => $callAppendFn($row, $appends));
      } else if ($data instanceof \Illuminate\Database\Eloquent\Model) {
         $callAppendFn($data, $appends);
      } else {
         throw new \Exception("Available append types didn't match for" . gettype($data));
      }
   }

   return $data;
}

// ...

$data = $this->addAppendsFix($request, $data);
mooseh commented 1 year ago

why not offer the ability to macro the package that way we can add our own approaches to this feature being removed?

abbluiz commented 6 months ago

It seems there are more ways of defining Attributes in Laravel 11 than creating " get...Attribute()" methods.