viezel / napp.alloy.adapter.restapi

RestAPI Sync Adapter for Titanium Alloy Framework
197 stars 102 forks source link

If params.url comes from model.url() errors occur #36

Closed wrightlabs closed 1 year ago

wrightlabs commented 10 years ago

(hello and great adapter!)

In my usage of this I have setup nested models/collections. For some of these, the model.config.URL cannot be used and is not there. (For instance 'http://myapi.com/v1/users/20/contacts' )

But my models do produce the correct url with the model.url() call.

The problem is without the changes I've proposed here my urls are as follows... create -> http://myapi.com/v1/users/20/contacts (correct) read -> http://myapi.com/v1/users/20/contacts/30/30 (incorrect) update -> http://myapi.com/v1/users/20/contacts/30/30 (incorrect) delete -> http://myapi.com/v1/users/20/contacts/30/30 (incorrect)

This pull request fixes the issue for me. I don't know if this is the best way to fix the problem, but I used this pull request to share the issue.

wrightlabs commented 10 years ago

I guess I should also note that this is regarding line 165

        params.url = (model.config.URL || model.url());
viezel commented 10 years ago

Hi @wrightlabs so great idea. Yeah Backbone is not very good for nested rest api. Im quite interesting in how you did solve that? if you care to share.

Your problem, how do you specify your url?

This is incorrect: (You should not include the id of the model in the url.)

myModel.save({
    id:30,
    title:" My Contact"
    ...
},{
    url: "http://myapi.com/v1/users/20/contacts/30"
});

If thats not how you are doing it, can you then please share the code.

Side comment: im not sure im very found of all the if(model.config.URL != null) logic. Think it could be written in a better way.

wrightlabs commented 10 years ago

@viezel sure I can share how I use Alloy (Backbone) with nested models/collections ... the models/users.js would be...

exports.definition = {  
    config: {
        "URL": "http://myapi.com/v1/users",
        "adapter": {
            "type": "restapi",
            "collection_name": "users",
            "idAttribute": "id"
        }
    },      
    extendModel: function(Model) {      
        _.extend(Model.prototype, {
             initialize: function() {
                this.contacts = Alloy.createCollection("contacts");
                this.contacts.user = this;
                this.urlRoot = this.config.URL;
              }
            });
        return Model;
    }, 
    extendCollection: function(Collection) {        
        _.extend(Collection.prototype, {
              url: function() {
                return this.config.URL;
              }
        });
        return Collection;
    }       
};

and model/contacts.js ...

exports.definition = {  
    config: {
        "adapter": {
            "type": "restapi",
            "collection_name": "contacts",
            "idAttribute": "id"
        },
    },      
    extendModel: function(Model) {      
        _.extend(Model.prototype, {
            });
        return Model;
    }, 
    extendCollection: function(Collection) {        
        _.extend(Collection.prototype, {
              url: function() {
                return this.user.url() + "/contacts";
              },
              setUser: function($user) {
                this.user = $user;
              }
        });
        return Collection;
    }       
};

(I think all this code works but I modified it to post it here)

Basically, I'm setting the urlRoot of the 'users' model (and overriding the url() method of the 'users' collection but I don't know if this matters), then I'm overriding the url() method of the 'contacts' collection (using this approach it will always be a 'users.contacts' collection).

From what I understand in Backbone, if an instance of the 'contacts' model is in the 'contacts' collection, then the 'contacts' model url() method return the correct url. (http://docs.appcelerator.com/backbone/0.9.2/#Model-url)

I came up with this approach based on the limited documentation they provide here ... http://docs.appcelerator.com/backbone/0.9.2/#FAQ-nested

Yes, I'm not surprised the if() logic is not appealing. I don't know any better way at the moment though.

viezel commented 10 years ago

Hi @wrightlabs Thanks for the showcase. Im not really sure this will benefit the general purpose of this adapter. The thing is that we need to alert the user if the URL config property is not set. (to help newbees)

wrightlabs commented 10 years ago

Ok, but doesn't line 165 assume that the model.config.URL might not be set?

        params.url = (model.config.URL || model.url());

I will see if I can come up with a better solution than the pull request when I get a chance.

viezel commented 10 years ago

not really see : https://github.com/viezel/napp.alloy.adapter.restapi/blob/master/restapi.js#L164-L170

It will display an error if the base url is not set, so either on the config or params.

viezel commented 10 years ago

or do I misunderstand what you mean?

wrightlabs commented 10 years ago

I see that an error will be displayed if "params.url" is not set.

But if I understand line 165 correctly, it says 'set "params.url" to "model.config.URL" or (if "model.config.URL" is not set) set "params.url" to the result of the "model.url()" method call.

I guess the issue is, in Backbone, "model.url()" returns the full URL of the model instance, regardless of what state it is in. For example if the model has the "id" parameter set it will return...

 http://myapi.com/v1/users/20/contacts/30

if the model id is not set , "model.url()" will return...

 http://myapi.com/v1/users/20/contacts
wrightlabs commented 10 years ago

I believe this example has solved my issue without changing the adapter code ...

exports.definition = {  
    config: {
        "URL": "http://myapi.com/v1/users",
        "adapter": {
            "type": "restapi",
            "collection_name": "users",
            "idAttribute": "id"
        }
    },      
    extendModel: function(Model) {      
        _.extend(Model.prototype, {
             initialize: function() {
                this.contacts = Alloy.createCollection("contacts");
                this.contacts.user = this;
                this.urlRoot = this.config.URL;
              }
            });
        return Model;
    }, 
    extendCollection: function(Collection) {        
        _.extend(Collection.prototype, {
              url: function() {
                return this.config.URL;
              }
        });
        return Collection;
    }       
};

and model/contacts.js ...

exports.definition = {  
    config: {
        "adapter": {
            "type": "restapi",
            "collection_name": "contacts",
            "idAttribute": "id"
        },
    },      
    extendModel: function(Model) {      
        _.extend(Model.prototype, {
             sync: function(method, model, opts) {
                  opts.url = this.collection.user.url() + '/contacts';
                  mod = require('alloy/sync/' + this.config.adapter.type);
                  mod.sync(method, model, opts);
              },
            });
        return Model;
    }, 
    extendCollection: function(Collection) {        
        _.extend(Collection.prototype, {
              url: function() {
                return this.user.url() + "/contacts";
              },
              setUser: function($user) {
                this.user = $user;
              }
        });
        return Collection;
    }       
};

This overrides the contacts model sync method and sets the 'opts.url', then calls the adapter sync method.