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

Implemented auto mapping, default schemas attributes and default hydrators attributes #47

Closed
wants to merge 3 commits into from

Conversation

pedroluislopez
Copy link

The modern frameworks do implement automatic features in order to avoid complex configuration files that are modified, usually, by several developers.

In order to improve this package, we develop two features. First avoid modification of json-api.php configuration file to include mappings of schemas and adapters: these are included automatically (we call this automapping). The second feature uses fillable attributes from eloquent models to avoid repetition of these attributes declaration in the schema and hydrator classes (by default, if attributes are declared in schema/hydrator class, these are used).

To use automapping you have to follow these steps:

  1. We have included a new configuration parameter in namespaces. Now, you can use automapping for several namespaces including automapping: true in their configuration. Example:
'namespaces' => [
    'v4' => [
        'automapping' => true,
        'url-prefix' => '/api/v4',
        'supported-ext' => null,
    ],
],
  1. The mapping model/schema is declared in each Laravel model. Laravel models should be in app/Models folder. Also, in the model you have to declare a constant: const JSON_API_SCHEMA = Schema::class;.

  2. The new implementation search in app/Models folder, classes that declare JSON_API_SCHEMA constant and generates automatic schema mappings for namespaces configured for this purpose, and also generates eloquent-adapter map using resourceType in schema classes.

  3. Automapping don't overwrite declarations of mappings and adapters declared in json-api.php configuration file.

Note: We have created a completely new class called CloudCreativity\LaravelJsonApi\Repositories\EloquentSchemasRepository because CloudCreativity\JsonApi\Repositories\SchemasRepository have their attributes declared as private, and we can't inherit them.

@lindyhopchris
Copy link
Member

@pedroluislopez thank you for the PR.

I'll have to take a look into this in detail as some of it is quite a big change. I really like the idea of getting the fillable attributes from the Model, so that definitely gets the thumbs up.

My quick question on this is does the auto-mapping also work if the API has a mixture of Eloquent and non-Eloquent models? It is essential that this package does not assume that all models are Eloquent because that is not the case in every single implementation we have.

@lindyhopchris
Copy link
Member

Also, my general thinking on API config is that it should be cacheable, i.e. it should be in a config file rather than automatically generated on the fly. This is for performance reasons: it seems costly to work this out on every single request when it can be cached into config.

Would be interested to hear what you think about automapping vs cached config in terms of performance?

@lindyhopchris
Copy link
Member

@jstoone as someone else who uses this package a lot I'd be interested in hearing your thoughts if you've got a spare few minutes

@pedroluislopez
Copy link
Author

pedroluislopez commented Feb 28, 2017

@lindyhopchris You're right, the solution with automapping would affect performance. Anyway, conceptually you receive a model you want its schema from, and this could be defined inside that model to later use reflection when asking for the schema to obtain it. This behavior would be also fast and it would be developed inside the class Neomerx\JsonApi\Schema\Container. This is a 3rd party library, so redefining this class in the json-api package should be explored. If you like, I could think about it and make you a PR in the future.

About the adapters, a similar behavior should be looked for but I'll need to evaluate it better.

If you like, I can cancel this PR and I send you another one including only the feature about using fillable attribute in eloquent models for attributes in the schema and the hydrator.

@lindyhopchris
Copy link
Member

@pedroluislopez yes, definitely send me a PR for the fillable thing - really like that.

Definitely let me know your thoughts when you've had a think about the Schema - maybe write an issue before spending the time writing code?

I do have a concern about reading it from the Model class though, which is that if you have two APIs in your application - e.g. v1 and v2 that represent the model in different ways in each. We already have that situation where we have a private API and a public API, which have some models that are exposed differently in each.

So I'm not totally convinced about reading it directly from the model class, but if your thoughts could cover that topic then i'd definitely be interested in what you come up with.

@pedroluislopez
Copy link
Author

@lindyhopchris
Attributes PR: #49
Use reflection for schemas PR: cloudcreativity/json-api#33

@pedroluislopez
Copy link
Author

@lindyhopchris
Adapters PR: #50

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

Successfully merging this pull request may close these issues.

2 participants