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

Record paths handle root-level array indices inconsistently #775

Closed
jdmnd opened this issue Aug 2, 2017 · 5 comments
Closed

Record paths handle root-level array indices inconsistently #775

jdmnd opened this issue Aug 2, 2017 · 5 comments

Comments

@jdmnd
Copy link
Contributor

jdmnd commented Aug 2, 2017

In the js client:

let r = client.record.getRecord('foo')
r.set([0,1,2,3])
r.get() // [0, 1, 2, 3]
r.set('[4]', 4)
r.set('.5', 5)
r.get() // [0, 1, 2, 3, 4, 5]
r.discard()
client.record.snapshot('foo', (err, record) => console.log(record)) // [0, 1, 2, 3, null, 5]

In the example above, local and remote record state diverge due to different handling of the path. The client believes the '[4]' path accesses the fourth index of the array, the server believes the path is invalid.

Perhaps this issue should be raised against the client instead, since it's supposed to replicate the behaviour of the server, but in this case I believe this is a due to a regression in server behaviour.

@jdmnd jdmnd added the bug label Aug 2, 2017
@jdmnd
Copy link
Contributor Author

jdmnd commented Aug 9, 2017

Related: #651

@derrandz
Copy link

It seems that in v4, the states no longer diverge, I've tested so with the same script as John's in v4:

const deepstream = require('deepstream.io-client-js')
const BBPromise = require('bluebird').Promise
const server = require('./server')

const expect = require('chai').expect

const run = async() => {
  await server.start()

  const client = deepstream('localhost:6020', {
    reconnectIntervalIncrement: 100,
    maxReconnectInterval: 1000,
    maxReconnectAttempts: 999,
  })

  await client.login({})

  const record = client.record.getRecord('posts/a')
  await record.whenReady()

  record.set([0, 1, 2, 3])
  await BBPromise.delay(500)

  record.set('[4]', 4)
  await BBPromise.delay(500)

  record.set('.5', 5)
  await BBPromise.delay(500)

  const localSnapshot = record.get() // [0, 1, 2, 3, null, 5]

  record.discard()
  await BBPromise.delay(500)

  const snapshot = await client.record.snapshot('posts/a')  // [0, 1, 2, 3, null, 5]

  let exceptionCaught = false
  try {
    expect(snapshot).to.deep.equal(localSnapshot)
  } catch (e) {
    exceptionCaught = true
  } finally {
    const message = exceptionCaught
      ? 'failed: path with array indices does not update values properly'
      : 'succeeded: path with array indices updates values correctly'

    console.log('Assertion', message)
    process.exit(exceptionCaught)
  }
}

run()

So this is fixed in v4 since both the local version of the record and the server's have the value [0, 1, 2, 3, null, 5],

However as you can see, the client does not handle properly root level array indices path, resulting in the null in the fourth position. Not sure if it is a client or server issue


You can find the script in: https://github.com/deepstreamIO/test-scripts/commit/8361b68fe79f6521fa7ffb0951dbb1a5d0a230ca

@AlexBHarley
Copy link
Contributor

In this script it doesn't look like you wait for the discardTimeout, so when you snapshot you're just loading the same data again.

I would recommend connecting another client and doing a snapshot to confirm data is the same between client and server

@derrandz
Copy link

Thank you Alex for pointing out the discard timeout option, that escaped my mind.

Here is the updated version of the script, which also reveals that the states no longer diverge, so we are only left with the issue of not being able to set a root level array using brackets notation, so statements like record.set('[4]', 4) fail to update the root level array correctly.

const deepstream = require('deepstream.io-client-js')
const BBPromise = require('bluebird').Promise
const server = require('./server')

const expect = require('chai').expect
const assert = require('./assert').assert

const run = async() => {
  const snapshots = {
    clientA: {
      local: null,
      remote: null,
    },
    clientB: {
      remote: null
    }
  }

  const discardTimeout = 1000

  await server.start()

  const clientA = deepstream('localhost:6020', {
    reconnectIntervalIncrement: 100,
    maxReconnectInterval: 1000,
    maxReconnectAttempts: 999,
    discardTimeout
  })

  const clientB = deepstream('localhost:6020', {
    reconnectIntervalIncrement: 100,
    maxReconnectInterval: 1000,
    maxReconnectAttempts: 999,
  })

  await clientA.login({})
  await clientB.login({})

  const record = clientA.record.getRecord('posts/a')
  await record.whenReady()

  record.set([0, 1, 2, 3])
  await BBPromise.delay(500)

  record.set('[4]', 4)
  await BBPromise.delay(500)

  record.set('.5', 5)
  await BBPromise.delay(500)

  snapshots.clientA.local = record.get()

  record.discard()

  await BBPromise.delay(discardTimeout)

  snapshots.clientA.remote = await clientA.record.snapshot('posts/a')
  snapshots.clientB.remote = await clientB.record.snapshot('posts/a')

  const assertion = assert(() => {
    expect(snapshots.clientA.local)
      .to.deep.equal(snapshots.clientA.remote)

    expect(snapshots.clientA.local)
      .to.deep.equal(snapshots.clientB.remote)
  }, {
      success: 'path with array indices does not update values properly',
      failure: 'path with array indices updates values correctly'
    })
  
  await BBPromise.delay(100)
  process.exit(assertion)
}

run()

@yasserf
Copy link
Contributor

yasserf commented May 3, 2020

Closing in favour of #1046

@yasserf yasserf closed this as completed May 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants