Add parameters to Remote Config template #479

lahirumaramba · 2020-09-25T21:37:03Z

Introduce Response Types and Public Types
Implement toResponseType() and toPublicType()
Add parameters to RemoteConfigTemplate
Add unit tests

Related Issue: #446

hiranya911

Looks pretty good. I suggested a few renames and test additions.

src/main/java/com/google/firebase/remoteconfig/ExplicitParameterValue.java

src/main/java/com/google/firebase/remoteconfig/FirebaseRemoteConfigClientImpl.java

src/main/java/com/google/firebase/remoteconfig/InAppDefaultValue.java

hiranya911 · 2020-09-29T18:28:11Z

src/main/java/com/google/firebase/remoteconfig/RemoteConfigParameter.java

+
+  public RemoteConfigParameter() {
+    conditionalValues = new HashMap<>();
+    defaultValue = ExplicitParameterValue.of("");


Why this default value?

If no default value is set (if defaultValue is null), the REST API sets the default value to an empty string. We can allow null here and add a null check in the conversion or just set it to an empty value in the ctor.

src/main/java/com/google/firebase/remoteconfig/RemoteConfigParameter.java

src/main/java/com/google/firebase/remoteconfig/internal/TemplateResponse.java

src/test/java/com/google/firebase/remoteconfig/FirebaseRemoteConfigClientImplTest.java

hiranya911

Looks great. Just a couple of suggestions.

hiranya911 · 2020-09-30T19:27:55Z

src/main/java/com/google/firebase/remoteconfig/RemoteConfigParameter.java

  public RemoteConfigParameter() {
    conditionalValues = new HashMap<>();
-    defaultValue = ExplicitParameterValue.of("");
+    defaultValue = RemoteConfigParameterValue.of("");


I feel like both these should remain null until explicitly set. It's ok to let the backend choose default values for these as necessary.

I see your point. Updated the code and added null checks in conversions.
I have a few concerns with setting these to null though. Isn't it recommended not to return null collections in Java? If we don't set to conditionalValues to an empty map at init, the following will throw an exception on the client side.

RemoteConfigParameter p = new RemoteConfigParameter().getConditionalValues().put("ios_en", ParameterValue.of("text")

This can be prevented with a simple null check on the client side, though.

hiranya911 · 2020-09-30T19:35:43Z

src/test/java/com/google/firebase/remoteconfig/FirebaseRemoteConfigClientImplTest.java

    assertTrue(parameters.containsKey("header_text"));
-    Map<String, RemoteConfigParameterValue> conditionalValues = parameters
-            .get("welcome_message_text").getConditionalValues();
+    RemoteConfigParameter parameter = parameters.get("welcome_message_text");


Also assert on header_text which has an in app default value param.

egilmorez

A couple suggested edits and a few questions, thanks!

egilmorez · 2020-09-30T20:12:20Z

src/main/java/com/google/firebase/remoteconfig/RemoteConfigParameter.java

+
+  /**
+   * Gets the conditional values of the parameter.
+   * The condition name of the highest priority (the one listed first in the


This is interesting . . . you mean this gets all the possible values, including the one actually assigned -- for a particular client instance? -- in the top position in the map?

This gets all the conditional values assigned for that specific parameter. You can set a default value in a parameter to be used when all the conditional values are evaluated to false. Otherwise, the parameter will use the conditional value with the highest priority. The priority is determined by the order of the conditions are stored in the top-level conditions array of the RemoteConfigTemplate. The top-level conditions array is not part of this PR and will be added in a future PR.
https://firebase.google.com/docs/reference/remote-config/rest/v1/RemoteConfigParameter

OK, thanks for the explanation. Parameter evaluation is pretty complex . . . when everything is done, I might try to start a review of our "parameters and conditions" concepts page, just to make sure it's all still accurate.

egilmorez · 2020-09-30T20:13:31Z

src/main/java/com/google/firebase/remoteconfig/RemoteConfigParameter.java

+
+  /**
+   * Sets the default value of the parameter.
+   * This is the value to set the parameter to, when none of the named conditions


Does the developer actually set the value? Or does our backend logic?

When getting an existing template the backend will set the values for existing Parameters. If the developer wants to create and add new parameters then they can set a default value to set the parameter to, when none of the named conditions evaluate to true.

egilmorez · 2020-09-30T20:15:32Z

src/main/java/com/google/firebase/remoteconfig/RemoteConfigParameter.java

+  }
+
+  /**
+   * Sets the conditional values of the parameter.


Again this is maybe just my own confusion, but -- I can see how a developer might want to automate the setting of a list of conditional values. But isn't the actual value sort of set at runtime, after evaluating the conditions?

When creating new parameters the developer needs to add conditional values manually. The condition names in conditionalValues map must match the names in the top-level conditions array in RemoteConfigTemplate. Otherwise, the API will throw a condition does not exist error.

egilmorez · 2020-09-30T20:20:40Z

src/main/java/com/google/firebase/remoteconfig/RemoteConfigParameterValue.java

+  }
+
+  /**
+   * Represents an in-app-default parameter value.


Suggest removing the hyphen between app and default.

egilmorez · 2020-09-30T20:31:07Z

src/main/java/com/google/firebase/remoteconfig/internal/TemplateResponse.java

+  }
+
+  /**
+   * The DTO for parsing Remote Config parameter value responses from the Remote Config service.


I wonder if we should spell out "Data Transfer Object" -- that's what this is, right? :)

I searched and couldn't immediately find a single other instance of "DTO" in docs/reference/

I think the classes in internal packages do not show up in the docs. I agree though Data Transfer Object makes more sense. I will update the comment here. Thanks!!

egilmorez

Looks good Lahiru, thanks!

hiranya911

The semantics aren't quite sitting well with me. But let's merge this and address any concerns later.

hiranya911 · 2020-10-01T22:23:39Z

src/main/java/com/google/firebase/remoteconfig/internal/TemplateResponse.java

      return new RemoteConfigParameter()
-              .setDefaultValue(defaultValue.toRemoteConfigParameterValue())
+              .setDefaultValue(remoteConfigParameterValue)
              .setDescription(description).setConditionalValues(conditionalPublicValues);


Nit: One setter per line

hiranya911 · 2020-10-01T22:25:29Z

src/main/java/com/google/firebase/remoteconfig/internal/TemplateResponse.java

+    public ParameterResponse(@Nullable ParameterValueResponse defaultValue,
+                             @Nullable String description,
+                             @NonNull Map<String, ParameterValueResponse> conditionalValues) {
+      checkNotNull(conditionalValues, "conditional values must not be null.");


Probably redundant at this level.

lahirumaramba force-pushed the lm-rc-parameters branch 3 times, most recently from 5e0dc25 to b5878b3 Compare September 28, 2020 19:06

Add parameters to template

fa23a4d

lahirumaramba force-pushed the lm-rc-parameters branch from b5878b3 to fa23a4d Compare September 28, 2020 19:08

lahirumaramba marked this pull request as ready for review September 28, 2020 19:10

lahirumaramba requested a review from hiranya911 September 28, 2020 19:10

lahirumaramba assigned hiranya911 Sep 28, 2020

lahirumaramba changed the title ~~Add parameters to template~~ Add parameters to Remote Config template Sep 28, 2020

Fix no op in conditional values

83c69db

lahirumaramba force-pushed the lm-rc-parameters branch from 1480b24 to 83c69db Compare September 28, 2020 20:43

Fix empty conditional values

c001c94

lahirumaramba assigned lahirumaramba and unassigned hiranya911 Sep 28, 2020

Introduce public and response types

625f514

lahirumaramba assigned hiranya911 and unassigned lahirumaramba Sep 29, 2020

lahirumaramba added 2 commits September 29, 2020 13:52

Cleanup unused imports

6ce6806

Init defaultValue in RemoteConfigParameter

dcb3856

hiranya911 suggested changes Sep 29, 2020

View reviewed changes

hiranya911 assigned lahirumaramba and unassigned hiranya911 Sep 29, 2020

lahirumaramba force-pushed the lm-rc-parameters branch from b48cf0e to 0dcf08e Compare September 30, 2020 19:19

Add javadocs and other PR fixes

0f5d2c7

lahirumaramba force-pushed the lm-rc-parameters branch from 0dcf08e to 0f5d2c7 Compare September 30, 2020 19:23

lahirumaramba assigned hiranya911 and unassigned lahirumaramba Sep 30, 2020

hiranya911 suggested changes Sep 30, 2020

View reviewed changes

hiranya911 assigned lahirumaramba and unassigned hiranya911 Sep 30, 2020

lahirumaramba assigned egilmorez Sep 30, 2020

egilmorez reviewed Sep 30, 2020

View reviewed changes

lahirumaramba added 2 commits October 1, 2020 13:22

Fix javadocs

9a80629

PR fixes

66ec860

egilmorez approved these changes Oct 1, 2020

View reviewed changes

lahirumaramba assigned hiranya911 and unassigned lahirumaramba and egilmorez Oct 1, 2020

Made all collections non-null

8aaebef

hiranya911 approved these changes Oct 1, 2020

View reviewed changes

hiranya911 assigned lahirumaramba and unassigned hiranya911 Oct 1, 2020

One setter per line

a4e8a42

lahirumaramba merged commit 150b121 into remote-config Oct 2, 2020

lahirumaramba deleted the lm-rc-parameters branch October 2, 2020 18:35

Add parameters to Remote Config template #479

Add parameters to Remote Config template #479

Uh oh!

Conversation

lahirumaramba commented Sep 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lahirumaramba Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

egilmorez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

egilmorez left a comment

Choose a reason for hiding this comment

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lahirumaramba commented Sep 25, 2020 •

edited

Loading

lahirumaramba Oct 1, 2020 •

edited

Loading