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

Resource idName #38

Closed
JeanLucEsser opened this issue Feb 15, 2017 · 5 comments
Closed

Resource idName #38

JeanLucEsser opened this issue Feb 15, 2017 · 5 comments
Labels

Comments

@JeanLucEsser
Copy link
Contributor

JeanLucEsser commented Feb 15, 2017

This is more of a clarification than an issue but still.

In the json-api config file, you can specify the column to use for the resource id. Or so I thought. I tried adding something like People\Schema::RESOURCE_TYPE => 'mynewid' with no luck.

The only way I could change the id used in the returned Json was by overriding theidName property in the resource Schema: protected $idName = 'mynewid';

This may be a newbie question, but did I misunderstood the use of the eloquent-adapter's columns array in the config file?

@lindyhopchris
Copy link
Member

Hi!

The Eloquent adapter is used by the store to checked that the combination of the type and id in JSON received from a client are valid. I.e. it validates inbound request content and the resource id that is in the URL.

The Schema is resposible for producing the JSON version of a resource - i.e. outbound content. The column used for the id is set on the idName property.

So if the People resource mapped to a Person Eloquent model, and mynewid was the column for the JSON-API id value, then you'd have to add People\Schema::RESOURCE_TYPE => 'mynewid' to your Eloquent adapter config, and in the People\Schema class set the idName property to mynewid.

Hope that helps.

@JeanLucEsser
Copy link
Contributor Author

JeanLucEsser commented Feb 15, 2017

Yes, it helps, but I still don't get it:

To validate inbound requests for a PATCH, you check that the resource type and id in the url api/v1/people/{idvalue} matches the type and id properties in the Json "type"="people","id"="idvalue". Those informations are constant and cannot be overridden says the specification. The Json will always have a type and id key.

The only reason I can think of is to check for existence of the resource in the database (PATCH), or for unicity (POST), via a validator, thus the column name could be mynewid. But then it brings another incoherence:

Why should we declare this column name twice? First in the Schema for creating the Json (outbound), and then via the config file for validating the resource to patch (inbound)? I can't think of a single scenario where those would be two different values?

At least default value for the Schema should be the one in the config file (if declared).

@lindyhopchris
Copy link
Member

For the URL id value, the store needs to check that a person object with that id exists, otherwise it needs to send a 404 response.

For example on content validation, if receiving this inbound POST request:

{
	"data": {
		"type": "comments",
		"attributes": {
			"content": "My comment..."
		},
		"relationships": {
			"posts": {
				"data": {
					"type": "posts",
					"id": "99"
				}
			}
		}
	}
}

The validators need to check the posts:99 exists. That's all done via the store that uses its adapters to work that out.

In terms of having to define it twice. That's because at the moment there's no central definition of how an Eloquent model maps to its JSON API representation. That's definitely the kind of direction that this package could go in. However, at the moment the schema and the store are separate things because the scheme comes from the underlying library that this package is built on - the neomerx/json-api package. That has an excellent serialization layer, but it doesn't handle anything else such as validating inbound content etc. Which is why I've had to add concepts such as the store on top.

As this package develops hopefully we can combine the Eloquent integration down into a single place rather than the separate places as exists at the moment. Any ideas and suggestions are welcome!

@JeanLucEsser
Copy link
Contributor Author

Yes your example illustrates my second paragraph, I get it. And I understand the need for separation. I will gladly participate and make suggestions, once I am more familiar with the package. Here's one: make default values configurable, such as createdAt = true. There's a good chance that when we don't want those timestamps included in a record, we don't want them in any, so it would be better not having to override every Schema (on second thought I could extend a MainSchema that does the overrides).

I may have more questions, is there a Gitter chat for the package to address questions from the community?

Needless to say, I plan on using this a lot and you did a marvelous job, thanx!

@lindyhopchris
Copy link
Member

@JeanLucEsser no gitter channel at the moment I'm afraid. For the mo, just open issues to ask questions.

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

No branches or pull requests

2 participants