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

Path Configuration Settings Block and Arrays 🤔 #209

Open
donnfelker opened this issue Dec 15, 2021 · 5 comments
Open

Path Configuration Settings Block and Arrays 🤔 #209

donnfelker opened this issue Dec 15, 2021 · 5 comments
Labels
enhancement New feature or request

Comments

@donnfelker
Copy link
Contributor

When the settings block of the path configuration json file contains an array GSON crashes with the following (example path configuration file below):

Caused by: java.lang.IllegalStateException: Expected a string but was BEGIN_ARRAY at line 6 column 14 path $.settings.
        at com.google.gson.stream.JsonReader.nextString(JsonReader.java:826)
        at com.google.gson.internal.bind.TypeAdapters$16.read(TypeAdapters.java:402)
        at com.google.gson.internal.bind.TypeAdapters$16.read(TypeAdapters.java:390)
        at com.google.gson.internal.bind.TypeAdapterRuntimeTypeWrapper.read(TypeAdapterRuntimeTypeWrapper.java:41)
        at com.google.gson.internal.bind.MapTypeAdapterFactory$Adapter.read(MapTypeAdapterFactory.java:187)
        at com.google.gson.internal.bind.MapTypeAdapterFactory$Adapter.read(MapTypeAdapterFactory.java:145)
        at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory$1.read(ReflectiveTypeAdapterFactory.java:131)
        at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory$Adapter.read(ReflectiveTypeAdapterFactory.java:222)
        	... 50 more

I see that this is because the TurboPathConfigurationSettings file only supports key/value pairs as per the docs:

/**
* Gets the top-level settings specified in the app's path configuration.
* The settings are map of key/value `String` items.
*/

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.

{
  "settings": {
    "screenshots_enabled": true,
    "register_with_account": true,
    "require_authentication": false,
    "tabs": [
      {
        "title": "Home",
        "path": "/",
        "image_name": "house"
      },
      {
        "title": "What's New",
        "path": "/announcements",
        "image_name": "megaphone"
      },
      {
        "title": "Notifications",
        "path": "/notifications",
        "image_name": "bell",
        "show_notification_badge": true
      }
    ]
  },
  "rules": [ ... ]
}
@jayohms
Copy link
Collaborator

jayohms commented Dec 15, 2021

@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.

@jayohms
Copy link
Collaborator

jayohms commented Dec 15, 2021

In the meantime, you could consider including string-escaped json in your settings value and deserialize the String value on your own.

@donnfelker
Copy link
Contributor Author

@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.

@donnfelker
Copy link
Contributor Author

@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

@jayohms
Copy link
Collaborator

jayohms commented Dec 21, 2021

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

@jayohms jayohms added the enhancement New feature or request label Feb 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

No branches or pull requests

2 participants