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

Enable custom 'key' field #238

Open
apuigsech opened this issue Feb 28, 2019 · 15 comments
Open

Enable custom 'key' field #238

apuigsech opened this issue Feb 28, 2019 · 15 comments
Labels
proposal A suggestion for change that has not been accepted

Comments

@apuigsech
Copy link
Contributor

The field used as key have to be 'id' right now. Would be great to be able to change this specific behavior and use a different field name as key.

@Dragomir-Ivanov
Copy link
Contributor

Can you explain what use-case this might help? Mongo also has fixed "id" field name _id.

@apuigsech
Copy link
Contributor Author

I am not using Mongo but a SQL database, so I am not linked to that limitation.

I a managing a set of objects that comes from an external entity that do not have 'id', but a 'name' field a uniq key. Right now I am using a dirty middleware to translate one schema to another (by renaming this specific field), but this is a solution I don't like at all.

@smyrman
Copy link
Collaborator

smyrman commented Mar 1, 2019

Personally I feel it's a reasonable limitation for rest-layer to require an ID field that is actually named `"id". In my mind, this enforces a more consistent API, and you never need to wonder about which field in a schema is used in URL lookups.

The MongoDB back-end renames queries against "id" to the field "_id". This is hard-coded, but there is no need for this to be hard-coded for an SQL back-end. I feel it would be best for the storage-backend to take an optional initialization parameter id column name. This could be done in several ways. E..g:

s := sqlstore.New(db *sql.DB)
s.SetIDColumn("id")
idx.Bind(...., s)

@Dragomir-Ivanov
Copy link
Contributor

Dragomir-Ivanov commented Mar 1, 2019

I am with @smyrman. Having id field as a standard is a good practice, and maybe rest-layer should keep that. DB Driver can be easily made to convert id to whatever, but I suspect @apuigsech would like to keep the same DB name on the API side. If there is only need for GET, there is a way to rename id to name with ?fields=*,name:id however PUT/POST is another thing.

@apuigsech
Copy link
Contributor Author

Okay, I can agree that there it may be good to have an specific field that refers to the key with consistency on its name.

Do you think it's possible to make this ID to be a clone of another field easily? Let's say that I want this 'id' field to be a clone of the 'name' field. How bad do you see that possible solution?

@Dragomir-Ivanov
Copy link
Contributor

I gave example for cloning for GET in my comment above. Do you need PUT/POST as well?

@apuigsech
Copy link
Contributor Author

apuigsech commented Mar 1, 2019

Yes, I need it for POST specifically.

This a non-working-code-yet that I am going to experiment with. Maybe it's an stupid solution, so comments are welcome :)

type FieldCopy struct {
	Source string
	Target string
}

func (fc FieldCopy)OnInsert(ctx context.Context, items []*resource.Item) error {
	for _,item := range items {
		item.Payload[fc.Target] = item.Payload[fc.Source]
	}
	return nil
}

func (fc FieldCopy)OnUpdate(ctx context.Context, item *resource.Item, original *resource.Item) error {
	item.Payload[fc.Target] = item.Payload[fc.Source]
	return nil
}

But I am getting this "2019/03/01 19:13:58 Server error: Missing ID field", because NewItem() checks the existence of the id field :(

@Dragomir-Ivanov
Copy link
Contributor

Can you specify how you are initializing this FieldCopy? id field is needed by rest-layer so must be in the payload.

@apuigsech
Copy link
Contributor Author

I do

r.Use(FieldCopy{Source: "name", Target: "id"})

And it works when I pass an empty "id" field, but not when it's not there.

@smyrman
Copy link
Collaborator

smyrman commented Mar 4, 2019

First of all, if you are able to modify the database to have a separate ID column, either a unique ID (e.g. xid) or a unique random ID (e.g. UUID v4), that would be the preferred solution in my opinion. This ID would be generated by the back-end on Create. Sometimes this isn't possible though, e.g. if the database is defined/designed outside your application.

There are at least two options you can try for allowing a blank id field:

  1. Make sure the "id" Field have Required set to false (i.e. not set to true). This is important because schema validation happens before Update or Create is called on the resource. Thus it happens before any hooks are called.
  2. If it still doesn't work as you want it to, and it's important that it works in this way, I would consider to handle this through a custom end-point, possibly with a customized URL if you don't want to override the REST-API one.

For 2, there are lots of pitfalls to be aware of, as in running Prepare/Validate is your responsibility. However you are able to modify and prepare the pipeline as you see fit. A good start would be looking at the code for the rest package.

We have a lot of cases where we do 2 for internal communication or complex tasks involving several resources (possibly external / non-rest-alyer ones), but the we do it via JSON RPC 2.0, and don't do anything REST-like.

@Dragomir-Ivanov
Copy link
Contributor

@apuigsech You are seeing this error, because id needs to be present in order for those resource.Items to be created in the first place. I guess apart from the @smyrman suggestion above, you can add the missing id fields before rest-layer takes control in a middleware(maybe alice as used in the rest-layer examples). I presume it will not be much more work, than you are doing in the Hooks. Also I guess, this is what you are doing already, so in any case you have specific use-case, and will need a workaround it.

@apuigsech
Copy link
Contributor Author

First of all, if you are able to modify the database to have a separate ID column, either a unique ID (e.g. xid) or a unique random ID (e.g. UUID v4)

The problem is that I am not able to change the contract, because it is already stablished and I don't have any control of the client-side, so I have to adapt to the server to it. The contract implements a very simple REST that using REST-layer can be developed quickly and in an elegant way. The only requirement is having a different key field.

Make sure the "id" Field have Required set to false (i.e. not set to true). This is important because schema validation happens before Update or Create is called on the resource. Thus it happens before any hooks are called.

This is exactly what I tried; Having a non-required id field, so the request is procesable, and then use Hooks to adapt it, so it's transparent for REST-layer. The problem is that there isn't a Hook executed before NewItem() is executed, and it has this code;

	id, found := payload["id"]
	if !found {
		return nil, errors.New("Missing ID field")
	}

If it still doesn't work as you want it to, and it's important that it works in this way, I would consider to handle this through a custom end-point, possibly with a customized URL if you don't want to override the REST-API one.

initially I don't like the idea as this affects to most of the endpoints the REST-layer creates, so at the end it's like do not use it, but I will evaluate this option more in depth.

Thank you so much for the ideas!

@apuigsech
Copy link
Contributor Author

@apuigsech You are seeing this error, because id needs to be present in order for those resource.Items to be created in the first place. I guess apart from the @smyrman suggestion above, you can add the missing id fields before rest-layer takes control in a middleware(maybe alice as used in the rest-layer examples). I presume it will not be much more work, than you are doing in the Hooks. Also I guess, this is what you are doing already, so in any case you have specific use-case, and will need a workaround it.

Yes, a Middleware is the only viable option I see right now.

Thanks!

@apuigsech
Copy link
Contributor Author

By the way, do you think that adding support to REST-layer for custom key field would be interesting? Something like;

	user = schema.Schema{
                KeyField: "email",                  // This is the field that will be used as Key. Default is 'id'.
		Fields: schema.Fields{
			"email": {
				Required: true,
				Filterable: true,
				Sortable:   true,
				Validator: &schema.String{}.
			},
			"created": schema.CreatedField,
			"updated": schema.UpdatedField,
			"name": {
				Required:   true,
				Filterable: true,
				Validator: &schema.String{},
			},
		},
	}

@markostojanovic087
Copy link

Hello guys, I am very interested in this question. Currently, I think I have a use case where it would be natural not to have a hardcoded 'id' as primary key. Due to a many to many relationship which forms associative entity in database, I want to use a composite primary key (made from foreigh keys) as it's primary key.

I am aware that I could solve my issue by having 'id' field as primary key and composite unique key for these foreign keys I have, although the 'id' field is logically redundant.

Since I am not so experienced, I would be grateful if you could share practical benefits of this practice with me. Besides the naming consistency and easier referencing in more complicated relationships (e.g. associative entity related to another entity forming another associative entity) are there any more advantages of the mandatory 'id' field? Thank you for your time!

@smyrman smyrman added the proposal A suggestion for change that has not been accepted label Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal A suggestion for change that has not been accepted
Projects
None yet
Development

No branches or pull requests

4 participants