Skip to content
This repository has been archived by the owner on Mar 29, 2022. It is now read-only.

Migrate to Targaryen 3 #100

Merged
merged 1 commit into from
Aug 19, 2017
Merged

Migrate to Targaryen 3 #100

merged 1 commit into from
Aug 19, 2017

Conversation

dotdoom
Copy link
Contributor

@dotdoom dotdoom commented Aug 8, 2017

  • fbRef is kept for its listener |on('value')| implementation
  • keeping _targaryen object around at all times, since setRules can be
    invoked at any time, and we need targaryen internal database to be
    up-to-date
  • tryXxx are now synchronous as they don't need to fetch value from
    fbRef

Suggest looking at the diff with "?w=1" (ignore whitespace diff)
appended to the GitHub URL.

@dotdoom
Copy link
Contributor Author

dotdoom commented Aug 8, 2017

For some story on it, I ran into an issue with Targaryen 2.3.3 with update (aka patch) and deep nodes, suspecting [1] and decided that it would be best for firebase-server to just upgrade.

[1] https://github.com/goldibex/targaryen/blob/v2.3.3/lib/ruleset.js#L308

@urish
Copy link
Owner

urish commented Aug 8, 2017

Thanks, Is that a breaking change in any way?

@dotdoom
Copy link
Contributor Author

dotdoom commented Aug 8, 2017

Hopefully not, but npm test and our own Android project tests are as much as I can be sure about that.

Nothing is expected to break according to their CHANGELOG.md - it is only listing a number of fixes.

BTW the issue I had with 2.3.3 is goldibex/targaryen#73.

@dotdoom
Copy link
Contributor Author

dotdoom commented Aug 16, 2017

A friendly ping!

@urish
Copy link
Owner

urish commented Aug 16, 2017

Have you seen my question above?

@dotdoom
Copy link
Contributor Author

dotdoom commented Aug 16, 2017

@urish just the one about whether it's a breaking change, but I've replied to it with as much information I have. Is there anything else I can do?

@urish
Copy link
Owner

urish commented Aug 18, 2017

@dotdoom regarding this line:

// BUG: tryRead() here, and if it throws, cancel the listener.

Can you explain?

@dotdoom
Copy link
Contributor Author

dotdoom commented Aug 19, 2017

@urish this is a finding unrelated to this PR.

An authentic Firebase server will cancel listeners server-side if a client no longer has a read permission to a specific node. This can happen either after a write to the database, or after a security rules change.

If we want to replicate a similar behavior with minimum effort, we could tryRead on every update to the node it's listening before sending an event down to the client. That's what BUG is about.

A better approach would be to store all the listeners and verify legibility of each of them on every write to the database, even if the write goes to a different node. That would replicate the exact behavior of Firebase server I've noticed in practice.

* fbRef is kept for its listener |on('value')| implementation
* keeping _targaryen object around at all times, since setRules can be
  invoked at any time, and we need targaryen internal database to be
  up-to-date
* tryXxx are now synchronous as they don't need to fetch value from
  fbRef

Suggest looking at the diff with "?w=1" (ignore whitespace diff)
appended to the GitHub URL.
@coveralls
Copy link

coveralls commented Aug 19, 2017

Coverage Status

Coverage increased (+0.5%) to 97.386% when pulling b044e92 on dotdoom:targaryen-3 into 1ae90a0 on urish:master.

@urish urish merged commit f34caf3 into urish:master Aug 19, 2017
@urish
Copy link
Owner

urish commented Aug 19, 2017

Got it, thanks!

@urish
Copy link
Owner

urish commented Aug 19, 2017

released as 0.11.0

Gabrn pushed a commit to Gabrn/firebase-server that referenced this pull request Aug 30, 2017
* fbRef is kept for its listener |on('value')| implementation
* keeping _targaryen object around at all times, since setRules can be
  invoked at any time, and we need targaryen internal database to be
  up-to-date
* tryXxx are now synchronous as they don't need to fetch value from
  fbRef

Suggest looking at the diff with "?w=1" (ignore whitespace diff)
appended to the GitHub URL.
p-salido pushed a commit to p/firebase-server that referenced this pull request Oct 3, 2017
* origin/master:
  chore: release 0.11.0
  chore: add package-lock.json
  doc: add a note about `-s` argument
  doc: add link to explain tryRead() issue
  Migrate to Targaryen 3 (urish#100)
  Support passing a WebSocket.Server options object to the FirebaseServer constructor (urish#101)
  Fix security rules validation for updates (urish#99)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants