Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

Allow null value for startAt, endAt and equalTo database queries on Android #504

Merged
merged 4 commits into from
Apr 23, 2018

Conversation

pulyaevskiy
Copy link
Contributor

This is a companion for #353 which fixes null value handling on Android side. There are some minor cleanups on iOS and Dart side as well.

Tested behavior of null startAt on Android and it works as expected.

@@ -83,7 +83,9 @@ private Query getQuery(FirebaseDatabase database, Map<String, Object> arguments)
Object startAt = parameters.get("startAt");
if (parameters.containsKey("startAtKey")) {
String startAtKey = (String) parameters.get("startAtKey");
if (startAt instanceof Boolean) {
if (startAt == null) {
query = query.startAt(null, startAtKey);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This invocation will become ambiguous, if another startAt overload with a reference type as first argument is ever added to the Firebase API. Since we actually invoke the startAt(String, String) method here, I suggest we collapse the null and String cases into one, handled by the else branch:

if (startAt instanceof Boolean) {
  query = query.startAt((Boolean) startAt, startAtKey);
} else if (startAt instanceof Number) {
  query = query.startAt(((Number) startAt).doubleValue(), startAtKey);
} else {
  query = query.startAt((String) startAt, startAtKey);
}

Similarly below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good point! Fixed.

@@ -70,7 +70,12 @@ - (NSDictionary *)dictionary {
}
id equalTo = parameters[@"equalTo"];
if (equalTo) {
query = [query queryEqualToValue:equalTo];
id equalToKey = parameters[@"equalToKey"];
if (equalToKey) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we not check for equality with [NSNull null] here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I simply updated this block to follow the same pattern as in startAt and endAt blocks where this check seems to be working as expected. I'm not an Obj-C expert by any means though.

This SO answer says that it's a right way to check for nil (which is a literal null value for Obj-C objects).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, because you also took care to change the Dart code to ensure that if there is an equalToKey entry in parameters, then it maps to a non-null value. I just didn't see that on my first reading.

@pulyaevskiy
Copy link
Contributor Author

This should be ready for another look.

Copy link
Contributor

@mravn-google mravn-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

query = query.equalTo(((Number) equalTo).doubleValue());
if (equalTo instanceof Boolean) {
query = query.equalTo((Boolean) equalTo);
} else if (equalTo instanceof String) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String case should be moved to else branch here too.

Copy link
Contributor

@mravn-google mravn-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM

@mravn-google mravn-google merged commit 8785770 into flutter:master Apr 23, 2018
@mravn-google
Copy link
Contributor

mravn-google commented Apr 23, 2018

Published. Thanks a lot for the contribution!

@pulyaevskiy
Copy link
Contributor Author

This was quick. :) Thanks for your help as well!

slightfoot pushed a commit to slightfoot/plugins that referenced this pull request Jun 5, 2018
julianscheel pushed a commit to jusst-engineering/plugins that referenced this pull request Mar 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants