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

multipath save #366

Conversation

olivierchatry
Copy link
Contributor

Test are not passing, because I think they are not valid in the way we want this thing to work :

To destroy a link you need to set null to it, and some of the tests are just emptying the relation object.

For example :

// comments is a hasMany
comments:{id1:true,id2:true}
// test want to reset comment and check if it was emptied
comments:{}

Cannot work, at it will either require comment to be set to null :

comments:null

Or all elements to be set to null:

comments:{id1:null,id2:null}

The other problem, which I'm not sure I can trace back ( lacking time ) is this error :

Error: Firebase.update failed: First argument contains an invalid key (children/- KAJmTkzssePlP9HbfSs) in property 'blogs.tests.adapter.updaterecord.embedded.treeNodes.-KAJmTkyP_fKXLpXTw47.children.-KAJmTkyP_fKXLpXTw48'. Keys must be non-empty strings and can't contain ".", "#", "$", "/", "[", or "]" (http://localhost:7357/assets/vendor.js:71731)

I think it is "internal" checking.

As an example here is what the multipath code generate from the usual serializedRecord :

// BEFORE ======> 
{
    "label": "Parent Node",
    "created": null,
    "children": {
        "-KAJmTkyP_fKXLpXTw48": {
            "id": "-KAJmTkyP_fKXLpXTw48",
            "label": "Child Node",
            "created": null,
            "children": {
                "-KAJmTkzssePlP9HbfSs": {
                    "id": "-KAJmTkzssePlP9HbfSs",
                    "label": "Child Sub Node",
                    "created": null,
                    "children": null,
                    "config": null
                }
            },
            "config": null
        }
    },
    "config": null
}
// AFTER ======> 
{
    "label": "Parent Node",
    "created": null,
    "config": null,
    "children/-KAJmTkyP_fKXLpXTw48": {
        "id": "-KAJmTkyP_fKXLpXTw48",
        "label": "Child Node",
        "created": null,
        "config": null,
        "children/-KAJmTkzssePlP9HbfSs": {
            "id": "-KAJmTkzssePlP9HbfSs",
            "label": "Child Sub Node",
            "created": null,
            "children": null,
            "config": null
        }
    }
}

@olivierchatry olivierchatry changed the title multipath generation, seems to work but test are not passing multipath save Feb 12, 2016
@olivierchatry
Copy link
Contributor Author

( I added output of "BEFORE / AFTER" in the test for debugging purpose )

@olivierchatry
Copy link
Contributor Author

@tstirrat I'm really not sure if it is the way to go. I'm saving the previously serialized object to be able to do the diff when updating, but this also needs to be maintained accross the different update ( which anyway happen only in _handleChildUpdate ). Feedback would be appreciated, if you know a better way.

There is also one test ( last one ) failing. I'm not sure what is the message is about.

At the very least the serialisation seems to be OK.

@olivierchatry
Copy link
Contributor Author

well, seems like I screwed the commit, I will push again on monday.

@olivierchatry
Copy link
Contributor Author

So, for my project ( which is not using embedded record ) everything works super fine.

But, the last test is failing, which is using embedded record.

Error: Firebase.update failed: First argument contains a path /children/-KAZcuPNcbNIVw0aul7- that is ancestor of another path /children/-KAZcuPNcbNIVw0aul7-/children/-KAZcuPNcbNIVw0aul70 (http://localhost:7357/assets/vendor.js:71762)

The errror is explicit, I'm not sure there is away to fix that, except doing multiple update in this particular case. Any thought @tstirrat ?

@olivierchatry
Copy link
Contributor Author

@tstirrat ping.

@tstirrat
Copy link
Contributor

@jamesdaniels and I are going to revisit this next week.

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

Successfully merging this pull request may close these issues.

3 participants