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

Protect GeoJSONFeature from ProGuard alterations #5813

Merged
merged 1 commit into from
Oct 31, 2023

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Oct 31, 2023

Closes #5812

Why is this the best possible solution? Were any other approaches considered?

We use com.fasterxml.jackson in javarosa to map geojson files into java classes. One of those classes is GeoJSONFeature class with its private Map<String, String> properties field. So properties are pairs where both keys and values should be strings. When it comes to keys it's not a problem because in geojson they are always just strings but values could be integers for example:
"properties": { "_uid_": 1,...
com.fasterxml.jackson takes care of that and automatically converts those values into strings because that's what we expect private Map<String, String> properties. However, if minification is enabled ProGuard messes up with that class disabling that conversion.
The fix is to let ProGuard know that this class should be intact.

Another solution would be to change private Map<String, String> properties to private Map<String, Object> properties and always programmatically convert those object values to strings. However, if com.fasterxml.jackson does that for us (if ProGuard does not break it) we should still take advantage of it I guess.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Testing the form shared on the forum (see the issue) should be enough. I can't be sure that we don't have other issues like this one caused by proguard but the fix itself is safe so we can focus on testing the described scenario.

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

  • added or modified tests for any new or changed behavior
  • run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
  • added a comment above any new strings describing it for translators
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

@grzesiek2010 grzesiek2010 added the high priority Should be looked at before other PRs/issues label Oct 31, 2023
Copy link
Member

@seadowg seadowg left a comment

Choose a reason for hiding this comment

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

Nice fix!

In general, it makes sense to keep classes for anything being serialised as it's really easy for ProGuard to mess up any meta programming magic. Maybe we should also keep GeojsonGeometry as well to defend against a similar problem in the future?

@grzesiek2010
Copy link
Member Author

In general, it makes sense to keep classes for anything being serialised as it's really easy for ProGuard to mess up any meta programming magic.

I was thinking about it and for example, introducing an annotation that we could later use in proguard to exclude classes that should not be changed (without listing all of them and just using the annotation) but I think it would be easy to forget either way so it doesn't make sense.

Maybe we should also keep GeojsonGeometry as well to defend against a similar problem in the future?

I'm not sure but we can do that.

@seadowg seadowg modified the milestone: v2023.3 Oct 31, 2023
@seadowg seadowg merged commit 7dff22a into getodk:v2023.3.x Oct 31, 2023
6 checks passed
seadowg added a commit to seadowg/collect that referenced this pull request Oct 31, 2023
Protect GeoJSONFeature from ProGuard alterations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high priority Should be looked at before other PRs/issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A form with a geojson attachment can't be opened: java.lang.integer cannot be cast to java.lang.string
3 participants