Open donnfelker opened 2 years ago
@donnfelker I agree that this is currently a weak spot in the path configuration. Let me think about this a bit more, because we need to provide backwards compatibility with existing settings
configs in the wild. We might need to leave settings
as simple key/value and add a new default block that we can let the app provide a deserializer or something similar.
In the meantime, you could consider including string-escaped json in your settings
value and deserialize the String
value on your own.
@jayohms Good point, maybe I could just do that for now (send down some escaped json). Thanks for the idea. Not optimal, but it will work until a solution is determined. Thanks.
@jayohms I think this also exposes a potential bug. If the settings are as above (tabs is an array), the app will crash. This exposes a potential issue with the configuration parsing. If the Server returns an invalid path config, it could crash the app.
What do you think about some sort of try/catch around that logic and then logging an error if it cannot parse?
This is the error that is thrown if the parsing bombs (and kills the app).
com.google.gson.JsonSyntaxException: java.lang.IllegalStateException: Expected a string but was BEGIN_ARRAY at line 1 column 110 path $.settings
Yep, that's a good call. There's no reason to crash the app if the remote config cannot be parsed. However, it might make sense to crash the app if the packaged local config parsing fails, since you shouldn't ship a build with that issue. I'll take a look at it after the new year.
I also recommend adding (at least) a smoke test on the Rails side. Something like:
test/integration/mobile_app_configuration_test.rb
require "test_helper"
# This is a sanity check to ensure the file exists, is accessible, and can be parsed
# it doesn't test it is *correct*
class MobileAppConfigurationTest < ActionDispatch::IntegrationTest
test "iOS: v1 configuration is accessible and parseable" do
get "/configurations/ios-v1.json"
assert_response :ok
assert response.parsed_body["rules"].count > 1
assert response.parsed_body["settings"].count > 1
end
test "Android: v1 configuration is accessible and parseable" do
get "/configurations/android-v1.json"
assert_response :ok
assert response.parsed_body["rules"].count > 1
assert response.parsed_body["settings"].count > 1
end
end
When the settings block of the path configuration json file contains an array GSON crashes with the following (example path configuration file below):
I see that this is because the
TurboPathConfigurationSettings
file only supports key/value pairs as per the docs:Is there a way to support arrays of objects in the settings block? It could even be a map of some sort so that it's generic.
I took a look at some custom GSON Deserializers but it was not immediately clear how to best model this so I wanted to run it by here. If not, how would you recommend we model this below in the path config?
The settings block has some code that looks like this, note the
tabs
array in the settings - this is the array that causes the problem.