Resource ID with value zero throws the exception: No JSON API resource id name set on route

[dacercoolex]

Trying to get a resource which ID value is zero, kinda similar to an issue found at the former cloudcreativity/laravel-json-api throws the LogicException No JSON API resource id name set on route. Turns out that on funtion modelOrResourceId in Route.php an assingnmet within the if stament is performed and it throws the exception because a logic value false is assumed, instead of the actual 0.

Here's referenced code (at line 102):

    /**
     * @inheritDoc
     */
    public function modelOrResourceId()
    {
        if (!$name = $this->resourceIdName()) {
            throw new LogicException('No JSON API resource id name set on route.');
        }

        if ($modelOrResourceId = $this->route->parameter($name)) {
            return $modelOrResourceId;
        }

        throw new LogicException('No JSON API resource id set on route.');
    }

And this is the workaround the I've implemented in order to prevent the exception being thrown:

    /**
     * @inheritDoc
     */
    public function modelOrResourceId()
    {
        if (!$name = $this->resourceIdName()) {
            throw new LogicException('No JSON API resource id name set on route.');
        }

        if (!is_null($this->route->parameter($name)) && $this->route->parameter($name)!='') {
            return $this->route->parameter($name);
        }

        throw new LogicException('No JSON API resource id set on route.');
    }

I'm not sure if this is the best solution but, considering that it's plausible to have a resource ID = zero, it solves my issues without affecting any other values.

Hope it helps!

[lindyhopchris]

Hi! Thanks for reporting, you are right that this is a bug that needs fixing. Are you able to do a PR?

I'd prefer to assign the value to a variable then test it in the if statement, and use the same test as was added in the linked issue. I.e.:

    public function modelOrResourceId()
    {
        if (!$name = $this->resourceIdName()) {
            throw new LogicException('No JSON API resource id name set on route.');
        }

        $modelOrResourceId = $this->route->parameter($name);

        if (!empty($modelOrResourceId) || '0' === $modelOrResourceId) {
            return $modelOrResourceId;
        }

        throw new LogicException('No JSON API resource id set on route.');
    }

I believe that test is ok, because the route parameter() method should return a string, object or null. (Object is returned if the binding has been substituted.) I don't believe it would ever return an integer.

[dacercoolex]

Hi, @lindyhopchris!

Yes, I think your approach is more elegant, assigning the route parameter to a variable, that way there's no need to recall the getter multiple times.

I've just tested it and it worked like a charm! So, if there's no problem, I'll send a PR including your approach.

In the other hand, I'm facing another issue related with this, but this time is around resource's validation, so far I've detected that idValidator class (validate and acccept functions), at the spec package, is leading to the same behavior, and so on, I'm still working because fixing it leaded me to hit another obstacle. Would you suggest that I should open another issue for it or could be just chained with this one and, therefore, PR both solutions?

By the way, thanks for your quick reply!

[lindyhopchris]

No that's ok, leave it on this issue rather than opening a new issue.

[dacercoolex]

Great! Thing is that I've just realized that it's part of the spec package, so I'd have to send the PR into that one.

[lindyhopchris]

You could maybe leave that one for me to sort out.

[dacercoolex]

It sounds good to me! I could help you just saying that you might have to implement the same solution on idValidator and ClientIdValidator, which is where I'm currently stuck because now I'm having issues updating records with Id zero (Patch method), but I'm still wondering why's that.

[lindyhopchris]

@dacercoolex there were several problems in the spec package with zero ids, which I believe I've fixed. Could you give it a go?

To bring in the fix, use the following:

composer require "laravel-json-api/spec:dev-hotfix/1.1.1 as 1.1.1"

If it is all working for you with that fix, I can tag the packages.

[dacercoolex]

Of course @lindyhopchris! Let me test it and I'll let you know the results.

[dacercoolex]

@lindyhopchris I made some test and, after some tweaking on validation rules of my resource's request, the issue was solved.

Thing is that, when value is zero, I had to ask if isUpdating and, if it's, bypass the id's required rule. I was looking into it and seams to be at the ResourceObject class.

Here's what I did to make it work:

    public function rules(): array
    {
        $model = $this->model();

        $rules = [
            'name' => ['required', 'array', 'min:1'],
        ];

        if (!$model) {
            $rules['id'] = ['required', JsonApiRule::clientId(), 'min:1', 'max:3'];
        }
        return $rules;
    }

Maybe there's a better way to accomplish this, but for testing purposes and on my implementation it was good enough to make everything work.

[lindyhopchris]

@dacercoolex glad to hear it worked.

You shouldn't have to do that for the validation rules. If you do this:

public function rules(): array
{
    return [
        'name' => ['required', 'array', 'min:1'],
        'id' => ['required', JsonApiRule::clientId(), 'min:1', 'max:3'],
    ];
}

Presumably it fails on update because your id of "0" fails the rule "min:1". I'm intrigued why you're doing min:1 when the whole point of this issue was you wanted to use zero as an id?!

[lindyhopchris]

Sorry, just twigged that you also mentioned the ResourceObject class... thanks for mentioning that, looking at the code I need to put a fix in there too.

[dacercoolex]

Thanks for taking the time to follow this @lindyhopchris !

You made a good point with your observation, but it would be spot on if the id were numeric, which is not my case, since values are filtered as string, so my intention with the rule is to force the string to have at least 1 character. It might be redundant being required, but I'll review it when I optimize the code during testing.

In any case, I'll test it removing the min rule in order to probe your theory.

Thanks again!

Presumably it fails on update because your id of "0" fails the rule "min:1". I'm intrigued why you're doing min:1 when the whole point of this issue was you wanted to use zero as an id?!

[dacercoolex]

I've tested it removing the min rule and it didn't work, still fails :-(

In any case, I'll test it removing the min rule in order to probe your theory.