search

rs / rest-layer

REST Layer, Go (golang) REST API framework
http://rest-layer.io
MIT License
1.26k stars 114 forks source link

Assist AutoIncrement FieldValidator with SQL backend #266

Open ishan1608 opened 4 years ago

ishan1608 commented 4 years ago

I have an auto incrementing integer field (ID) in postgres db.

Whole creating an object using POST request I have to provide OnInit hook with myself manually handling the values. Instead the better approach would be to let the db handle the ID generation and give me the newly generated ID for the new object.

Is it possible in the current system? If yes how? If no should we add this feature? Also if no what is the solution for now other than making a db query myself and finding out the largest ID and adding 1 to it?

I have posted a StackOverflow question with more details here: https://stackoverflow.com/q/59734290/1641882

smyrman commented 4 years ago

There is no specific support for Auto-Incremental IDs in rest-layer itself. However I belive a resource.Storer implementation can alter the passed in items (e.g. populate an ID field) on insert operations.

The SQL/Postgre SQL storer backend is thirdparty, and not maintained by us. It's probably best to start the implementation of such a feature there.

Otherwise, be sure to understand the limitations of auto incremental IDs, and when (not) to use them:

If you don't have a specific use-case where Auto-Incremental IDs are essential, I would at least personal recommend against using them.

ishan1608 commented 4 years ago

I don't have control over the schema to change the ID from auto increment to uuid. However, I wasn't looking for Auto-Incremental ID support in rest-layer, instead what I was looking for is a way to specify exclusion of the ID field when creating/inserting objects through POST request.

I was thinking of forking the repo, adding the functionality of field exclusion from POST requests only. Then I will use it for my use case and create a PR for the same, so that if you are happy with the changes, then maybe rest-layer can support it. Can you point me to the right direction for the same?

smyrman commented 4 years ago

The SQL backend I know about is here:

A good starting point may be to fork that and start trying out different things.

You can start experimenting with saving an item without an ID, and see where it fails using the resource layer directly.

If it fails in the storage backend, then that would be a good place to try to implement AutoIncremental ID support. If it fails in rest-layer, we can look at what changes are needed to allow for an empty ID.

An alternative to an empty ID, could also to define a "dummy" value (in the OnOnit field hook), that the storer backend ignores or translates into a relevant. Such a field-type could be defined in the storer backend.

ishan1608 commented 4 years ago

I was running out of time and needed to show something, so I hacked my way to using auto incrementing integers with https://github.com/apuigsech/rest-layer-sql I am now able to make POST requests and rows are getting created in the DB, however I am facing a more fundamental problem. If I make a GET request to this path /api/users/4 I am getting this error:

{
    "code": 500,
    "message": "not an integer"
}

I tracked the problem and found this: We assume the ID to be string here https://github.com/rs/rest-layer/blob/0fa67ca486a972bd2ee8837c3c2591c4909858c4/rest/routing.go#L97 Then we try to validate the ID with the field ID here https://github.com/rs/rest-layer/blob/0fa67ca486a972bd2ee8837c3c2591c4909858c4/rest/resource_path.go#L57 which fails because in the beginning only we kept the id as string and not int.

I have noticed two assumptions (correct me if I am wrong): a) ID is a string b) ID is randomly generated ID and not database generated

For my use case I need these two features supported: a) ID can be integer and not just string b) Support for fields whose values are generated at the DB level.

I understand the fact that this requires changes in Storer interface https://github.com/rs/rest-layer/blob/0fa67ca486a972bd2ee8837c3c2591c4909858c4/resource/storage.go#L10 which will break compatibility with pre-existing third party packages

I understand that these are fundamental changes and would require lot of effort and careful considerations. I was hoping to use rest-layer in production but because of my restrictions of not being able to change the schema it doesn't looks feasible to use it right now.

Having said that, I am more than willing and happy to contribute and make all the changes required to support the above mentioned features. However, I will need your help and guidance to get this done.

I am hoping that if you agree and we work to get these features supported, then I will be able to use it in my production.

smyrman commented 4 years ago

a) ID is a string.

ID is always a string when read from the URL as opposed to read from JSON, where it can have different types. To fix this, you can try to let the ID field validator support a "string" input and convert it to an integer (basically write your own FieldValidator). After an ID is parsed, there should be no further assumptions requiring the ID to be a string.

b) ID is randomly generated ID and not database generated.

There is no assumption that it is generated. However, it's possible that there could be some places where it's assumed that the ID field is set before the resource.Item reaches the storer backend (even when the field is not set as Required) -- you just need to check that ( I don't remember all parts of the code good enough to give a definitive answer). If you are able to do a POST as you describe, then it's a good indication that not having an ID is actually allowed.

Either way there should be no need of changing the Storer interface to support this. the storer interface is passed pointers to resource.Items, and if these items don't contain an ID during an Insert operation then just set one. It should be encoded back into the response if it's set.

ishan1608 commented 4 years ago

There indeed are places where it is assumed that ID field is set as you have already pointed in b). Right now I am doing these: a) Using a dirty hack of fetching the max ID from database in the OnInit function. b) Forked rest-layer-sql and i) added support for filtering on integer fields ii) converting int64 to int while setting item.ID

This setup gets me going.

Going forward, I will create PRs for both rest-layer and rest-layer-sql to add support for ID to be integer and database generated fields.

Thanks for all the help that you have provided.

P.S.: If you wish we can close this issue and open another one once I have created the PRs and/or need help. Alternatively we can label this issue appropriately to indicate that initial issues are resolved and more work is expected in future.

smyrman commented 4 years ago

Here is a possible less hacky approach you can try (add this field definition to your rest-layer-sql fork). PS! Haven't looked up all the definitions, so the code below is liekely to contain errors. Check the docs to adapt/complete it.

type autoIncrType uint8

const (
    AutoIncr autoIncrType = iota // Dummy value set in rest-layer that we can detect in the backend.
)
type AutoIncrInt64Field = schema.Field{
    Required: true,
    Oninit: func() {return AutoIncr},
    FieldValidatior: &AutoIncrInt64Validator{},
}

type AutoIncrValidator struct{}

func (*AutoIncrValidator) Validate(v interface{}) (out interface{}, error) {
    var i int64
    switch vt := v.(type) {
    case  autoIncrType {
       // Let SQL backend detect and remove this..
       return v, nil
   case string:
        i = ... // parse vt to int64
    case int64:
        i = vt
    case int32:
        i = int64(vt)
    default:
        return nil, ErrNotAnIntger
    }
    return i, nil
}

You can imagine a similar type for int32 or other integer type if relevant.

smyrman commented 4 years ago

Let's keep it open for now @ishan1608, and you can keep asking your questions here. Relabeled renamed it.