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

Updates for supporting the Android API #350

Merged
merged 4 commits into from
Oct 30, 2017

Conversation

johnjaylward
Copy link
Contributor

@johnjaylward johnjaylward commented Jun 21, 2017

Key Changes:

  • Reverts some of the recent entrySet uses in classes that are not in the Android API implementation
  • Changes the XMLTokener "skipPast" API to match the parent class JSONTokener API that Android uses
  • Moves the valueToString implementation from JSONObject to JSONWriter. The JSONObject.valueToString method is still there, but now calls the JSONWriter implementaion.

If this is accepted, we should be able to release a secondary JAR, or Android users can roll their own JAR that does not include the following files:

  • JSONArray
  • JSONException
  • JSONObject
  • JSONStringer
  • JSONTokener

This should prevent any class loader issues between the 2 projects.

What problem does this code solve?
This removes API incompatibilities from our implementation to better match whats available in the Android implementation.

Risks
Medium. One public API in XMLTokener was changed to not have a return value. The superclass of XMLTokener is JSONTokener which is also included in Android. The Android version of JSONTokener has a public method of the same name, but with a void return. In order to have the XMLTokener extend the Android version of JSONTokener, the return values should match.

The risk here is delegated as "Medium" because it's unlikely that anyone is actually calling the method in question. it's very much an internal method that should probably never have been made public in the first place.

Also to note, the Android version of JSONException has the following 2 incompatibilities:

  1. Only a single constructor that takes a string.
  2. Still extends Exception instead of RuntimeException

In order to better match the Android version of JSONException I updated some method signatures in JSONPointer to include the exception cases.

I left comments in the code where incompatible JSONException instances are made. I did not change our implementation of JSONException.

I did submit a change request to Android to extend the JSONException class to have more constructors to match our API. The constructors may be a non-issue on future versions of Android. See https://android-review.googlesource.com/#/c/420159/
This change has been approved and merged into Android Master and should be part of the next android API release.

Changes to the API?
Noted in the Risks section.

Will this require a new release?
No, can be rolled into the next release

Should the documentation be updated?
No

Does it break the unit tests?
No. All unit tests should pass.

Was any code refactored in this commit?
Yes

Review status
ACCEPTED
Starting 3 day window for comments.

@stleary
Copy link
Owner

stleary commented Jun 21, 2017

Will this change allow Android users to do XML parsing?
Looks like tests are breaking in CookieListTest, JSONMLTest, and XMLtest.

@johnjaylward
Copy link
Contributor Author

yeah, it should allow XML parsing. I had a few errors, I'm fixing them now.

@johnjaylward
Copy link
Contributor Author

ok, I fixed the errors in the code here, and also needed to update the tests to have correct position information in the syntax errors. The implementation on Master has bad position information as it's expected cases.

@stleary
Copy link
Owner

stleary commented Jun 21, 2017

@AppWerft @quelgar @AvinashChowdary @powerqian
Any interest in taking this code for a test drive, to see if it works for you on Android apps?

@johnjaylward
Copy link
Contributor Author

This should be ready for live testing now. I believe I corrected all the wrong positions in any error messages.

As noted in the main comment, if used with Android, my changes are expecting you to NOT use the classes in this library that Android already provides. My changes should allow you to use the XMLTokener and XML classes directly with the Android org.json namespace.

@johnjaylward
Copy link
Contributor Author

I rebased this pull request to depend on #352 in case we want to fix the error message position errors first.

@johnjaylward
Copy link
Contributor Author

rebased back to master now that Tokener has been updated

@stleary
Copy link
Owner

stleary commented Oct 15, 2017

@johnjaylward do you still want to proceed with this? Do you recommend releasing an Android-only jar?

@johnjaylward
Copy link
Contributor Author

johnjaylward commented Oct 15, 2017 via email

@stleary
Copy link
Owner

stleary commented Oct 17, 2017

I will reach out to @BGehrels for a checkpoint release prior to the Android fix. Thought I had already opened an issue.

@stleary
Copy link
Owner

stleary commented Oct 20, 2017

@johnjaylward 20171018 is released and published on the Maven repo. OK to proceed if you want.

@johnjaylward
Copy link
Contributor Author

Updated to Master.

This should be compatible with Android 8.0 (API level 26) with no issues as long as the overlapping classes are removed.

It will be mostly compatible with Android API<=25 as long as no exceptions are thrown that include the "cause" exception. in those cases a MethodNotFoundException is thrown instead of the JSONException.

@stleary
Copy link
Owner

stleary commented Oct 28, 2017

Accepted, starting 3 day window.

@stleary stleary merged commit 722003d into stleary:master Oct 30, 2017
@stleary
Copy link
Owner

stleary commented Oct 30, 2017

@johnjaylward can you recommend how an Android dev might go about using these changes? What to include in the jar, any issues they should be aware of, etc?

@johnjaylward johnjaylward deleted the AndroidSupport branch October 30, 2017 16:34
@johnjaylward
Copy link
Contributor Author

johnjaylward commented Oct 30, 2017

Android Compatibility:

  • This should be compatible with Android >=8.0 (API level >=26) with no issues.
  • It will be mostly compatible with Android API<=25 as long as no exceptions are thrown that include the "cause" exception. In those cases a MethodNotFoundException is thrown instead of the JSONException.

How to use as a JAR:

  • Compile all sources
  • Package all compiled sources to a new JAR except the following (example jar name may be json-extensions-android.jar):
    • JSONArray
    • JSONException
    • JSONObject
    • JSONStringer
    • JSONTokener
  • Include the new JAR in your android project.

How to use from source

  • Create a new package in your Android project called org.json
  • Copy all the source files to this new package except
    • JSONArray
    • JSONException
    • JSONObject
    • JSONStringer
    • JSONTokener
  • Compile and run your project.

This will then use the built in Android JSON implementation and allow you to use JSONPointer, the XML tools, etc.

@stleary
Copy link
Owner

stleary commented Oct 30, 2017

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants