knuckleswtf / scribe

Generate API documentation for humans from your Laravel codebase.✍
https://scribe.knuckles.wtf/laravel/
MIT License
1.75k stars 314 forks source link

Models created by factory are deleted before calling the resource #56

Closed PollieDev closed 4 years ago

PollieDev commented 4 years ago

What happened? This is is what I assume is the problem, I could be wrong. Apologies if that's the case.

Some endpoints of ours have a model which creates multiple relations with the use of factories. In our case we have an application which belongs to a stage. Which then again belong to a job. So in our application factory we create a stage if one is not defined. And in the stage factory we create a job if one is not defined.

The creation of the items are working perfectly but once the model (the application) hits our resource the job doesn't exist anymore. The stage does still exist and works perfectly.

Example code:

Application Factory

$factory->define(Application::class, function (Faker $faker, $params) {
    if (isset($params['stage_id'])) {
        $stage = Stage::findOrFail($params['stage_id']);
    } else {
        $stage = factory(Stage::class)->create();
    }

    $stage->job // exist just as needed!

    return [
        // Attributes
    ];
});

Stage Factory

$factory->define(Stage::class, function (Faker $faker, $params) {
    if (isset($params['job_id'])) {
        $job = Job::with('company.jobsite')->findOrFail($params['job_id']);
    } else {
        $job = factory(Job::class)->create();
    }

    $job // Job exist perfectly fine

    return [
        // Attributes
    ];
});

Resource

class Application extends JsonResource
{
    /**
     * Transform the resource into an array.
     *
     * @param  \Illuminate\Http\Request  $request
     * @return array
     */
    public function toArray($request): array
    {
         $this->stage // exist just as expected.
         $this->stage->job_id // also exist and has the ID that was provided in the factory
         $this->stage->job // does not exist. We use this relation on many places so we are pretty sure it's not the relation
         Job::where('id', $this->stage->job_id)->first() // + this also returns null
    }
}

This is the docblock on the controller (if you'd need that information):

    /**
     * Retrieve an application
     *
     * @header X-Application-Access-Token {access_token}
     * @apiResource \App\Http\Resources\Content\Application
     * @apiResourceModel \App\Models\Application
     *
     * @param integer $id
     * @return \App\Http\Resources\Content\Application
     */

So it looks like the job has already been removed before I could make use of it.

My environment:

My Scribe config (minus the comments):

Config ```php 'laravel', 'static' => [ 'output_path' => 'public/docs', ], 'laravel' => [ 'add_routes' => true, 'docs_url' => '/docs', 'middleware' => [], ], 'auth' => [ 'enabled' => true, 'in' => 'bearer', 'name' => 'Authorization', 'use_value' => env('SCRIBE_AUTH_KEY'), 'extra_info' => 'extra info.', ], 'intro_text' => 'intro', 'example_languages' => [ 'bash', 'javascript', 'php', ], 'base_url' => env('API_URL'), 'title' => null, 'postman' => [ 'enabled' => false, 'base_url' => null, 'description' => null, 'auth' => null, ], 'default_group' => 'Endpoints', 'logo' => 'app/img/logo-docs.svg', 'router' => 'laravel', 'routes' => [ [ 'match' => [ 'domains' => [ env('API_SUBDOMAIN', 'api') . '.' . env('APP_TLD') ], 'prefixes' => [ 'content/v1/*', ], 'versions' => ['v1'], ], 'include' => [ // 'users.index', 'healthcheck*' ], 'exclude' => [ // '/health', 'admin.*' ], 'apply' => [ 'headers' => [ 'Content-Type' => 'application/json', 'Accept' => 'application/json', ], 'response_calls' => [ 'methods' => [], 'config' => [ 'app.env' => 'local', // 'app.debug' => false, ], 'cookies' => [ // 'name' => 'value' ], 'queryParams' => [ // 'key' => 'value', ], 'bodyParams' => [ // 'key' => 'value', ], 'fileParams' => [ // 'key' => '/home/me/image.png', ], ], ], ], ], 'fractal' => [ 'serializer' => null, ], 'faker_seed' => null, 'strategies' => [ 'metadata' => [ \Knuckles\Scribe\Extracting\Strategies\Metadata\GetFromDocBlocks::class, ], 'urlParameters' => [ \Knuckles\Scribe\Extracting\Strategies\UrlParameters\GetFromUrlParamTag::class, ], 'queryParameters' => [ \Knuckles\Scribe\Extracting\Strategies\QueryParameters\GetFromQueryParamTag::class, ], 'headers' => [ \Knuckles\Scribe\Extracting\Strategies\Headers\GetFromRouteRules::class, \Knuckles\Scribe\Extracting\Strategies\Headers\GetFromHeaderTag::class, ], 'bodyParameters' => [ \Knuckles\Scribe\Extracting\Strategies\BodyParameters\GetFromFormRequest::class, \Knuckles\Scribe\Extracting\Strategies\BodyParameters\GetFromBodyParamTag::class, ], 'responses' => [ \Knuckles\Scribe\Extracting\Strategies\Responses\UseTransformerTags::class, \Knuckles\Scribe\Extracting\Strategies\Responses\UseResponseTag::class, \Knuckles\Scribe\Extracting\Strategies\Responses\UseResponseFileTag::class, \Knuckles\Scribe\Extracting\Strategies\Responses\UseApiResourceTags::class, \Knuckles\Scribe\Extracting\Strategies\Responses\ResponseCalls::class, ], 'responseFields' => [ \Knuckles\Scribe\Extracting\Strategies\ResponseFields\GetFromResponseFieldTag::class, ], ], 'routeMatcher' => \Knuckles\Scribe\Matching\RouteMatcher::class, ]; ```
shalvah commented 4 years ago

After creating your job and stage in the factory, how do you attach them to the models?

PollieDev commented 4 years ago

In the application factory I do the following:

$factory->define(Application::class, function (Faker $faker, $params) {
    if (isset($params['stage_id'])) {
        $stage = Stage::findOrFail($params['stage_id']);
    } else {
        $stage = factory(Stage::class)->create();
    }

    return [
        'stage_id' => $stage->id,
        // Attributes
    ];
});

So I don't directly use eloquent relationships to attach them. I simply provide the ID of the relation when inserting the model. This is the same when creating a stage, a stage only belongs to 1 job so thereby I also provide the job_id directly when creating the factory.

shalvah commented 4 years ago

Hmmm, complicated. See if you can investigate some more. I don't have the resources right now to do so.

Worth noting that you're attaching your relationships in a somewhat "unconventional" manner. Have you tried switching to something like afterCreating()? Also using Eloquent relationships to attach?

ilgala commented 4 years ago

I've faced the same problem and I've found that is related to the UseApiResourceTags class. I think that class requires a little refactor for more than a reason:

At first I've implemented my version of that class for one reason: the route is passed in the constructor but the resulting "fake" request doesn't resolve that route, it simply returns an empty object. I have an API resource that does this check:

public function toArray($request) {
    if($request->named('what.so.ever') {
        // Format the response in this way
    } else {
        // Format the response in this other way
    }
}

And, since the request was empty, the application was throwing me an error so I've updated the __invoke method:

public function __invoke(
        Route $route,
        ReflectionClass $controller,
        ReflectionFunctionAbstract $method,
        array $rulesToApply,
        array $alreadyExtractedData = []
    ) {
        $this->route = $route;
        $docBlocks = RouteDocBlocker::getDocBlocksFromRoute($route);
       // ...
}

And the getApiResourceResponse method:

public function getApiResourceResponse(array $tags) {
    // ...

    $response = $resource->toResponse(app(Request::class)->setRouteResolver(function () {
        return $this->route;
    }));

    return [
        [
            'status' => $statusCode ?: 200,
            'content' => $response->getContent(),
        ],
    ];
}

That's the first point. The second one is related to the Issue 46 I've opened about 3 months ago. After reading your issue I've understood that these two things we're related and, after a deeper investigation, I realized that the package when it creates the model instances

  1. Opens a transaction
  2. Creates the instances from factories
  3. Closes the transaction and rolls back everything

In this way, if you try to lazy load a relation in your resource model it will return null since the relation doesn't exists anymore because of the roll back!

In order to make it work, I've updated both the instantiateApiResourceModel and getApiResourceResponse. I've removed the methods $this-startDbTransaction() and $this->endDbTransaction() from the first method, and moved to the second one. This is the result:

public function getApiResourceResponse(array $tags)
{
    $this->startDbTransaction();

    if (empty($apiResourceTag = $this->getApiResourceTag($tags))) {
        return;
    }

    // [...]

    $this->endDbTransaction();

    return [
        [
            'status' => $statusCode ?: 200,
            'content' => $response->getContent(),
        ],
    ];
}
shalvah commented 4 years ago

Thanks for the detailed analysis.

At first I've implemented my version of that class for one reason: the route is passed in the constructor but the resulting "fake" request doesn't resolve that route, it simply returns an empty object.

Yeah, the API Resource tags strategy basically just calls the method. It's ResponseCalls that initialises a request and does the full thing.

Sounds like you know the problem and the solution. Why not send in a PR?

ilgala commented 4 years ago

@shalvah I'm working on it! I've created a local feature branch and I'm studying also the rest of the code in order to make the necessary updates as well as test them! I'll keep you updated

shalvah commented 4 years ago

Fixed in #104 (1.9.0)