Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancements to API Resource Strategy #663

Closed
ncatanchin opened this issue Dec 29, 2019 · 3 comments
Closed

Enhancements to API Resource Strategy #663

ncatanchin opened this issue Dec 29, 2019 · 3 comments
Labels
next Relevant for the next major release

Comments

@ncatanchin
Copy link
Contributor

Enjoying the recently added support for Eloquent API Resources! I have extended this strategy locally to work around some issues / potential shortcomings.

Models being persisted to database

Found data being added every time docs are generated. This is due to my factories creating related models, eg. the Project factory does something like:

return [
    'user_id' => factory(App\User::class),
]

The API Resource Strategy uses factory()->make() to avoid persisting the model, but in my case related models are still persisted.

Incomplete model data

Related to the above - my models use Eloquent events to perform common actions. One example is assigning a UUID to each record. In order to have this UUID shown in the API docs, the model needs to be persisted.

My API resources often include other information, such as model counts of related models - eg. a users_count for each ProjectResource.

Solution - Use database transactions along with factory()->create()

The above issues can be resolved by using the same processes as in the ResponseCalls strategy.

  1. Copy methods from ResponseCalls:
    /**
     * @return void
     */
    private function configureEnvironment()
    {
        $this->startDbTransaction();
    }

    /**
     * @return void
     */
    private function finish()
    {
        $this->endDbTransaction();
    }

    /**
     * @return void
     */
    private function startDbTransaction()
    {
        try {
            app('db')->beginTransaction();
        } catch (\Exception $e) {
        }
    }

    /**
     * @return void
     */
    private function endDbTransaction()
    {
        try {
            app('db')->rollBack();
        } catch (\Exception $e) {
        }
    }

Ideally these methods would be moved to the Strategy (Extracting/Strategies/Strategy.php) abstract class.

  1. Use database transaction / rollback to avoid persisting any data

Add to __invoke() method:

        $this->configureEnvironment();

Tweak getApiResourceResponse() method:

            /** @var Response $response */
            $response = $resource->toResponse(app(Request::class));
            $response = [
                [
                    'status' => $statusCode ?: $response->getStatusCode(),
                    'content' => $response->getContent(),
                ],
            ];
        } catch (\Exception $e) {
            echo 'Exception thrown when fetching Eloquent API resource response for ['.implode(',', $route->methods)."] {$route->uri}.\n";
            if (Flags::$shouldBeVerbose) {
                Utils::dumpException($e);
            } else {
                echo "Run this again with the --verbose flag to see the exception.\n";
            }

            return null;
        } finally {
            $this->finish();
        }

        return $response;
  1. Change instantiateApiResourceModel() to instead use ->create():
            return factory($type)->create();

Further considerations

The configureEnvironment method in ResponseCalls strategy:

        $this->setEnvironmentVariables($rulesToApply['env'] ?? []);
        $this->setLaravelConfigs($rulesToApply['config'] ?? []);

This same functionality may also be useful to be supported in UseApiResourceTags.

Factory states

My factories make use of states which allow me to, eg:

  • Implement the most basic model for the default factory, with only required fields set
  • Build upon this base using states
  • Generate different types of models - User types / roles, models with many relations, invalid models - useful for tests

I could see it being useful to allow specifying the factory state that the strategy should use, or even having a default state that the strategy looks for, with a set name such as docs, apidocs:

$factory->state(App\User::class, 'docs', function (Faker $faker) {
    return [
        'name' => 'customised for',
        'email' => '[email protected]',
    ];
});

Alternatively, an annotation such as @apiResourceState could be the preferred solution

Related issues

The following issue is potentially related to the above:

#642 Eloquent API Resource with Loaded Relation


Have logged the above here for discussion or in case it helps anyone, hoping to put together a PR in the next week or two, time permitting

@shalvah
Copy link
Contributor

shalvah commented Jan 5, 2020

Sure, send in a PR.

Re database transactions, that's alright, but I'm not sure that database transaction method actually works for all DB connections (tbh, not sure it works at all).

Also, I;d favour adding parameters to existing annotations over adding new annotations (ie @apiResource User states=docs).

@shalvah shalvah added the next Relevant for the next major release label May 2, 2020
@shalvah
Copy link
Contributor

shalvah commented May 7, 2020

All of these are now supported in Scribe 🙂.

@shalvah shalvah closed this as completed May 7, 2020
@shalvah
Copy link
Contributor

shalvah commented May 13, 2020

Check out the docs on API resources.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next Relevant for the next major release
Projects
None yet
Development

No branches or pull requests

2 participants