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

Update the 'fromJSON' method of 'KrollDict' to map JSONObject.NULL to null. #2318

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@twaddington

twaddington commented Jun 4, 2012

Currently the KrollDict.fromJSON method ignores JSONObject.NULL values and inserts them into the Map unmodified. This is generally not a huge deal, but if you try to pass the resulting KrollDict instance to the Javascript layer via a KrollFunction callback, the JNI layer crashes:

https://gist.github.com/2dd449f4fbb9e6c5b7f3

I'm not sure if you're going to like this patch, but I thought I'd take a stab at it. I'm unsure if any tests would need to be modified, because I'm not familiar with your development stack.

Looking for feedback.

Update the 'fromJSON' method of 'KrollDict' to map the 'JSONObject.NU…
…LL' Object to a real Java null representation.
@negupta

This comment has been minimized.

Show comment
Hide comment
@negupta

negupta Jul 1, 2012

Contributor

@twaddington - I do not see a signed CLA with this account. Did you sign a CLA?

Contributor

negupta commented Jul 1, 2012

@twaddington - I do not see a signed CLA with this account. Did you sign a CLA?

@twaddington

This comment has been minimized.

Show comment
Hide comment
@twaddington

twaddington Jul 2, 2012

@negupta I haven't signed anything. I'm not sure what your legal process involves for submitting patches, just trying to help get this bug fixed.

twaddington commented Jul 2, 2012

@negupta I haven't signed anything. I'm not sure what your legal process involves for submitting patches, just trying to help get this bug fixed.

@negupta

This comment has been minimized.

Show comment
Hide comment
@negupta

negupta Jul 3, 2012

Contributor

@twaddington - Please take a look at the following link for the pull request process:
http://docs.appcelerator.com/titanium/2.1/index.html#!/guide/Pull_Request_Guide

Contributor

negupta commented Jul 3, 2012

@twaddington - Please take a look at the following link for the pull request process:
http://docs.appcelerator.com/titanium/2.1/index.html#!/guide/Pull_Request_Guide

@twaddington

This comment has been minimized.

Show comment
Hide comment
@twaddington

twaddington Jul 3, 2012

@negupta thanks! I'll read through that today.

twaddington commented Jul 3, 2012

@negupta thanks! I'll read through that today.

@negupta

This comment has been minimized.

Show comment
Hide comment
@negupta

negupta Jul 25, 2012

Contributor

@twaddington - Please let us know if you plan to sign the CLA.

Contributor

negupta commented Jul 25, 2012

@twaddington - Please let us know if you plan to sign the CLA.

@twaddington

This comment has been minimized.

Show comment
Hide comment
@twaddington

twaddington Jul 25, 2012

@negupta I looked over the CLA and honestly I just don't have the time to get everything set up and working right now. I don't think I'll be submitting any other patches, so it kind of fell off my plate. Perhaps someone else who has signed the CLA can accept this as a bug report and write a similar patch?

twaddington commented Jul 25, 2012

@negupta I looked over the CLA and honestly I just don't have the time to get everything set up and working right now. I don't think I'll be submitting any other patches, so it kind of fell off my plate. Perhaps someone else who has signed the CLA can accept this as a bug report and write a similar patch?

@negupta

This comment has been minimized.

Show comment
Hide comment
@negupta

negupta Jul 26, 2012

Contributor

We cannot review and accept any piece of code without a legal signed agreement. Closing this PR.

Contributor

negupta commented Jul 26, 2012

We cannot review and accept any piece of code without a legal signed agreement. Closing this PR.

@negupta negupta closed this Jul 26, 2012

@twaddington

This comment has been minimized.

Show comment
Hide comment
@twaddington

twaddington Jul 26, 2012

That's unfortunate. I know this is going to bite someone else in the future.

twaddington commented Jul 26, 2012

That's unfortunate. I know this is going to bite someone else in the future.

@AppWerft

This comment has been minimized.

Show comment
Hide comment
@AppWerft

AppWerft Sep 22, 2016

I run (maybe) in the same issue. I'm using https://github.com/stleary/JSON-java to generic convert xml into json. This works, but I run in an issue if try to convert into KrollDict for callback to JS layer.

AppWerft commented Sep 22, 2016

I run (maybe) in the same issue. I'm using https://github.com/stleary/JSON-java to generic convert xml into json. This works, but I run in an issue if try to convert into KrollDict for callback to JS layer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment