dobtco / formbuilder

[Needs Maintainer] Formbuilder is a small graphical interface for letting users build their own webforms.
http://dobtco.github.io/formbuilder/
MIT License
1.84k stars 488 forks source link

Duplicate cids when creating with bootstrapData #123

Open larryweya opened 10 years ago

larryweya commented 10 years ago

Steps to reproduce:

In a nutshell, whenever a Bakcbone object (Collection/Model or View) is created, Backbone uses _.uniqueId to generate a new client id and is assigned as the objects cid.

Formbuilder uses the cid as the forms unique identifier. When you restore the schema from bootstrap data, the existing cid is ignored by Backbone and a new one is created. Since everytime a page is loaded, the count re-starts from 1 an already existing id will likely be generated and attached to a new field. Ideally, if the same steps used to create the schema are used to restore it, the ids should never duplicate but the problem is that while restoring, the fields are created all at once while when creating a field is created with a number of views so the cid is incremented further when creating than when restoring.

The fix that I propose (and I'm working on) to to generate a unique id specifically for each field and that formbuilder keeps track of. Whenever a field is created, we'll check the highest number from the existing id's and increment from that. The same will be done when bootstraping. This will however break any existing schemas since we cant use the cid attribute to store the unique id, because Backbone ignores it a creates its own.

smurfpandey commented 9 years ago

I can confirm this issue.

larryweya commented 9 years ago

@smurfpandey while working on the above fix, I realized that after each save, you need to attach a unique id attribute and return the list of fields from the request. Formbuilder uses the id to assign cids if it doesn't find an id, it assumes its a new form and duplicates can occur. This is how I do it from a PHP script

$payload = json_decode(Request::instance()->getContent(), true); //Request::instance()->getContent() contains the request body sent by formbuilder
$payload['fields'] = array_map(function($field) {
    $field['id'] = $field['cid']; // set the id to the value of cid
    return $field;
}, $payload['fields']);
$record_set->schema = json_encode($payload);
$record_set->save();
return Response::json($payload['fields']); // return application/json of the fields array/list
smurfpandey commented 9 years ago

@larryweya I think the problem will still persist.

I did some digging, and i found that, every backbone model gets it cid using underscore's uniqueId method. This method increments a global variable and returns it's value as id. http://underscorejs.org/docs/underscore.html#section-140

Now the problem i see is, even if we add a "id", and existing models are assigned that id as "cid". All new models will still generate their cids using the _.uniqueId, which will start from 0/1, for every session.

This is what I think should happen in theory. What are your thoughts?

SongTanmay commented 9 years ago

First of all, good to know that there is someone who can understand my pain. I am going through same issue and its nightmare. At times there were instances that we have to work late night to fix something in related formbuilder and it came out to be same/duplicate cid issue.

As @smurfpandey mentioned .uniqueID is responsible for creating cids. When I was debugging this is who I came to know: deepmodel >> backbone model >> underscoreJS >> .uniqueID.

IMHO: _.uniqueID cannot be buggy as it is used by many libraries and no one has ever complained. Duplicate cid issue has to do something with our application, our way of using formbuilder.

Our project structure: We have integrated formbuilder with rails server. There is concept of asset pipeline in rails, which means that rails server loads all the required javascripts, css, images etc at the of loading. And on client side we have used requireJS to load dependency. When I was debugging this issue I found that underscoreJS is loaded twice, once from rails and second time from requireJS. Interestingly there were two physical underscorejs files present at a time or in other words _uniqueID was there twice. And I think this is the issue and global variable is referred twice with different value in it. May be I am wrong, correct me if I am? Now its loaded only from rails and we have not yet encountered this issue yet.

@larryweya Is this your case? or can you tell how is your application structure?

Guys please share your thoughts ?

smurfpandey commented 9 years ago

@SongTanmay Only real solution that I can think of is to not rely on "cid" at all. When your form is being saved on the server, you will have to assign a "id" to each control. Make sure id is only provided to controls that do not have one. And that id should be unique in that form.

So something like this will happen:

  1. Form is created first time with 2 controls, you server assign them a new id, c1, c2.
  2. That form is edited, and a new control is added at the bottom, you will loop through each control on and assigns the newly created control a new id: c3.

Boundary cases: What if existing control is removed, or a new control is added in between existing control.

You can also keep the logic of assigning the ids on the client end. Just make sure it's unique in a form.

SongTanmay commented 9 years ago

We are directly saving payload as string in database and then feeding payload directly to formbuilder at the time editing form. I think looping through each control will be costly if form has 120 fields in it and may be more than that.

larryweya commented 9 years ago

@smurfpandey I also found that setting the id when the form is saved stopped the duplication from happening on subsequent edits

@SongTanmay I'm also saving the payload to a database but after looping and assigning an id to each

smurfpandey commented 9 years ago

@SongTanmay In that case you should do it on client side, when ever a new model is added, just assign a new id too. The "cid" is added here. Just append your logic of assigning "id" to that method, and it should work.

SongTanmay commented 9 years ago

Yep I have given it a thought once that generating a random number and appending it to cid or simply adding time stamp to cid. Ya its a good workaround but is that the solution of the problem ?

larryweya commented 9 years ago

@SongTanmay I had started working on setting the id on the client side, the diff is here https://github.com/larryweya/formbuilder/compare/client_side_id, don't quite remember what was working but maybe it could help you

smurfpandey commented 9 years ago

Don't use random number, coz it's random. ;) Instead use the solution provided by @larryweya. It looks good to me.

And yes, this is the only solution it seems. This is how "cid" works in backbone, it will be unique for a particular session only. It's the responsibility of the developer to generate unique IDs for a model.

larryweya commented 9 years ago

@SongTanmay The issue does still exist if you still prefix your fields with a c because backbone will create models with the same prefix so if you already have a c2 in your schema a new c2 will be created and yours will be overridden. Th solution is to to always use custom server side ids if a field doesn't yet have an id

smurfpandey commented 9 years ago

@larryweya I think client side IDs will also work, you will just have to change this line

model.attributes.cid = model.cid

in main.coffee, instead of using model.cid use a custom function.

This way you don't need to parse the formbuilder JSON at server side, just dump it in your database if you want. But if you are storing the form in completely normalize form in your database, then server side IDs will work.

SongTanmay commented 9 years ago

@larryweya I like the idea of using getNextFieldId and finding the nextId. I am looking forward to use that as well. But can't I just do the same thing with cids instead for f+id or model.id? OR in another words can I do same thing with cids ?

larryweya commented 9 years ago

@SongTanmay I think the issue with that approach is that when using bootstrap data, when Backbone creates models, it assigns incrementing cids and if it comes across your bootstraped model with a cid it already assigned, your model will not be created, it will assume its a duplicate. You shoudl be able to use the model.id as long as its not prefixed with a c

KhaledSMQ commented 9 years ago

Hi, @larryweya @SongTanmay

i have solve this issue buy providing UUID for each element once i load data from DB to keep it as ID for the field , The form Builder on save event will look if element has CID if not will create new CID in order inside the DOM.

Please verify.


success: function(data) {
                    var datum, _i, _len, _ref5;
                    _this.updatingBatch = true;
                    for (_i = 0, _len = data.length; _i < _len; _i++) {
                        datum = data[_i];
                        if ((_ref5 = _this.collection.get(datum.cid)) != null) {
                            _ref5.set({

                                id: datum.id
                            });
                        }
                        _this.collection.trigger('sync');
                    }
laxmi-narayan-nitdgp commented 9 years ago

@KhaledSMQ
Can you provide lib code ??