Attempt at fixing issue with getting next and previous push id

[mortenbekditlevsen]

This is an attempt at fixing #8790. It's not very polished yet, and the test is currently too slow (loops over all single unicode character strings).

[mortenbekditlevsen]

Please note that this is only marked as WIP since I would very much like feedback on the solution.
Like:

1. Do you agree with the comment about adding key validation to the input of the successor and predecessor methods?
2. Does the overall strategy seem sound.

If you agree with the solution: Should I file issues for the other platforms that contain similar assumption errors in their successor and predecessor calculations with reference to this PR? Or will you take this up internally in some cross-platform firebase forum? :-)

[jmwski]

Thanks for this! I think this is a reasonable solution.

It looks like some FIRDatabaseQueryTests are failing in endBefore. Is that just in CI or do those tests fail locally as well?

[jmwski]

Looks like style checks are failing

Please note that this is only marked as WIP since I would very much like feedback on the solution. Like:

1. Do you agree with the comment about adding key validation to the input of the successor and predecessor methods?
2. Does the overall strategy seem sound.

If you agree with the solution: Should I file issues for the other platforms that contain similar assumption errors in their successor and predecessor calculations with reference to this PR? Or will you take this up internally in some cross-platform firebase forum? :-)

It's probably a good idea to add key validation.

You can file issues pointing at this PR in the other platforms. This is a reasonable solution -- the internal firebase forum consensus is only needed for the API not the implementation.

[mortenbekditlevsen]

It looks like some FIRDatabaseQueryTests are failing in endBefore. Is that just in CI or do those tests fail locally as well?

It appears that those tests are not being run through SwiftPM, that's the reason I've missed them. Not really certain about how to run them, but I'll go and have a look...

[mortenbekditlevsen]

@IanWyszynski , thank you very much for your review. I've implemented your suggestions - the only thing I haven't gotten around to yet is the tests that are not being executed using SwiftPM (which I use in my dev environment). I'll have a look at the latest test output from CI and see if I can find out what goes wrong by looking at that output.

[mortenbekditlevsen]

@IanWyszynski would you do a formal review of the PR, or would it be better if someone not involved closely with the details did that?

[paulb777]

Please update https://github.com/firebase/firebase-ios-sdk/blob/master/FirebaseDatabase/CHANGELOG.md with a note for 8.10.0 with a summary of the fix.

[mortenbekditlevsen]

Another small thought about this API:
Since it requires so much pretty specific unicode/utf16 'knowledge' to be built in to the clients, it might actually be prettier if the API was implemented server-side, so all clients don't have to re-implement this rather complex logic.

I am guessing that the API was added directly to the client since it does provide good value, and it seemed like an easy thing to add to the clients without involving the server. But as it turns out that it's not so easy, perhaps it would make sense for the server to provide the API out-of-the-box.

Just my two cents - knowing absolutely nothing about the server side implementation and the difficulties of adding a new API there, so please take this as a very general comment. :-)

[mortenbekditlevsen]

As an addition to my comment above (that it might be better to implement the start after / start before queries server-side), I realized that the implementation in this PR might actually still not be accurate.

If two keys are integer based, then the textually longest key comes after the shorter.
This means that the next key after 12345 is not 12346, but 012345.
And the next key before 12345 is 00000012344 (prefix zero-padding to max-32bit length (11 characters)).
I haven't tried it out, but I assume that the similar situation holds for negative numbers, but here we are on the negative axis, so it may be non-obvious that -01 comes after -1 and 01 comes after 1.

I haven't tested how -00000 sorts compared to 000, but I would assume from the algorithm that -00000 comes after 000... Hmmm.

This is a really hard thing to do right.

@IanWyszynski @paulb777

[mortenbekditlevsen]

Hmm, I just got the thought that there is actually an undefined sort order in the Obj-C and Javascript implementations of key sorting that I have seen.

'-0' and '00' have the same length and the same numerical value, but they are not the same. What is their sort order?

The RTDB console appears to be hiding my 00 key since there was already a -0 value... This looks like a bug, but I don't know how pervasive it is...

Hmm, I guess internally 00 and -0 resolves to the same key internally... That's a bit confusing.

Playing around in the console I get the following:

I add a key "-0" with the value "minus zero"
Then I add a key "00" with the value "double zero"
The console does not update with my "00" key/value, but still shows "-0": "minus zero"
If I refresh the UI then I see the "00": "double zero" value and not the "-0" key.
If I try accessing the "-0" key directly I see the "double zero" value.

My conclusion is that "-0" and "00" resolve to the same key (as does "-00" and "000", etc.)

[mortenbekditlevsen]

Now you see it, now you don't:

{
  "rules": {
    "00": {
    ".read": true,
    ".write": true
    },
    "-0": {
    ".read": false,
    ".write": false      
    }
  }
}

Works in the simulator at least.

[mortenbekditlevsen]

I think I found a difference in the key sort between ios and js:

In js there can be any number of 0's prefixed to a 32-bit int:

export const INTEGER_REGEXP_ = new RegExp('^-?(0*)\\d{1,10}$');

export const tryParseInt = function (str: string): number | null {
  if (INTEGER_REGEXP_.test(str)) {
    const intVal = Number(str);
    if (intVal >= INTEGER_32_MIN && intVal <= INTEGER_32_MAX) {
      return intVal;
    }
  }
  return null;
};

On iOS the length of the string is checked before conversion is attempted, so any string longer than 11 characters is never considered to be an integer value.

static inline BOOL tryParseStringToInt(__unsafe_unretained NSString *str,
                                       NSInteger *integer) {
    // First do some cheap checks (NOTE: The below checks are significantly
    // faster than an equivalent regex :-( ).
    NSUInteger length = str.length;
    if (length > 11 || length == 0) {
        return NO;
    }
    ...

I haven't yet tested which of the two implementations the server appears to use.

[mortenbekditlevsen]

Now I've tested, and it appears that the javascript implementation matches the server implementation in this regard. Should I file an issue about this for the iOS implementation?

Android appears to have the same issue:

  // NOTE: We could use Ints.tryParse from guava, but I don't feel like pulling in guava (~2mb) for
  // that small purpose.
  public static Integer tryParseInt(String num) {
    if (num.length() > 11 || num.length() == 0) {
      return null;
    }
   ...
}

[paulb777]

Thanks @mortenbekditlevsen

@IanWyszynski Would you follow up on the open questions and the review?

[mortenbekditlevsen]

@IanWyszynski @paulb777
Let me know if there's anything I can do to help here.
Should I continue to add fixes on top of the existing fixes? Or would you find it more appropriate to try and implement these APIs (the query after and before, I mean) in the backend somehow?
And what about the key sorting issue I pointed out above? Should I file a separate issue for that?

[jmwski]

@IanWyszynski @paulb777 Let me know if there's anything I can do to help here. Should I continue to add fixes on top of the existing fixes? Or would you find it more appropriate to try and implement these APIs (the query after and before, I mean) in the backend somehow? And what about the key sorting issue I pointed out above? Should I file a separate issue for that?

Given the complexity of getting this right, I'm leaning towards moving startAfter/endBefore to the backend so we at least don't have to maintain a separate implementation in each SDK. @jsdt wdyt?

[paulb777]

In email exchange with @IanWyszynski and @schmidt-sebastian, we agreed that even if a better long-term solution is a back end fix, this is a good interim solution.

[paulb777]

@mortenbekditlevsen Anything outstanding to resolve before merging? I just tried the new test and the speed seems fine.

It does look like the https://github.com/firebase/firebase-ios-sdk/blob/master/FirebaseDatabase/CHANGELOG.md still needs an update.

[mortenbekditlevsen]

It's still missing a fix for next/prev of an integer key prefixed by a number of zeroes.

[mortenbekditlevsen]

I'll look into fixing that part.

[schmidt-sebastian]

Thanks for this. Please let us know if you have time to address the feedback.

[mortenbekditlevsen]

Ok, I have briefly considered the fix for 'integer' base key ordering.
I think that inconsistencies between the server key sorting and the [FUtilities compareKey:toKey:] sorting needs to be fixed first, because otherwise it's not clear which ordering this fix should be against - and if it should use the server ordering, then we can't use [FUtilities compareKey:toKey:] to test it.

So here are some things to consider:

1. Fix up [FUtilities compareKey:toKey:] to not only consider keys of character length less than 11 to be integer keys - in order to align with server algorithm. I think I noted somewhere upthread that this appears to be an issue in the Android SDK too, so someone should warn that team about the inconsistency too.
2. Introduce special casing for the fact that the two keys "-0" and "00" are in fact the same key (the sort equally and can be used interchangeably). Similarly for "-00" and "000", etc. up to max key length.
3. Next key after any positive integer key is "0"+key - until the key length is max key length, then the integer is incremented.
4. Next key after any negative integer key is "-0" + (negated) key
5. Next key after 0 is 00 (or "-0", but I guess the former is the more obvious choice)
6. Next key after a 'negative 0' '-0' is also either '000' or '-00', but I feel like 'normalising' to a sequence of zeroes is probably better.
7. Previous key after any positive integer (without zero prefix padding) is the integer value before, prepended with the appropriate amount of zeroes to make the key of length 'max key length'.
8. Ditto for negative integers
9. For zero prefixed integers (or '-0') then one 0 can just be removed.
10. For zero, the previous integer is -00...001
11. For values like "-0000" I also suggest normalising to the equivalent key "00000" before decrementing to the previous value "0000".

Does that sound about right?
@schmidt-sebastian @paulb777 Would you comment about the suggested fix for [FUtilities compareKey:toKey:] and let me know if I should do anything about the similar issue in the Android SDK?

[schmidt-sebastian]

cc @jsdt and @IanWyszynski for comments on the ordering in compareKey.

[jmwski]

There is an outstanding conversation here, but I think we can go ahead and merge this since it fixes the immediate issue. I plan to re-implement startAfter/endBefore on the server side to avoid introducing this complexity in all the different SDKs, but I haven't been able to prioritize that work yet.

[paulb777]

Thanks @IanWyszynski I resolved to unblock the merge and merged.

[mortenbekditlevsen]

Should I create an issue to track the discrepancy with key sorting on iOS and Android vs js and server? @paulb777 @IanWyszynski