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

Refactor vector to list #22

Closed

Conversation

anildahiya
Copy link
Contributor

No description provided.

@mikeburke106
Copy link
Contributor

I will review in more detail, but I'm guessing this change will break the library due to reflection in the JsonRPCMarshaller:

public static JSONObject serializeHashtable(Hashtable<String, Object> hash) throws JSONException{
    JSONObject obj = new JSONObject();
    Iterator<String> hashKeyIterator = hash.keySet().iterator();
    while (hashKeyIterator.hasNext()){
        String key = (String) hashKeyIterator.next();
        Object value = hash.get(key);
        if (value instanceof RPCStruct) {
            obj.put(key, ((RPCStruct) value).serializeJSON());
        } else if (value instanceof Vector<?>) {
            obj.put(key, serializeVector((Vector) value));
        } else if (value instanceof Hashtable) {
            obj.put(key, serializeHashtable((Hashtable)value));
        } else {
            obj.put(key, value);
        }
    }
    return obj;
}

This method is performing reflection, looking for a Vector. Since the vectors were refactored into lists, this check will fail and the result of toString() on the list object will be stored in the JSONObject (obviously, not correct). This is one reason reflection shouldn't be used like this.

I will verify and review further, but I'm guessing this pull request should be rejected until the JsonRPCMarshaller class is refactored as well.

@anildahiya
Copy link
Contributor Author

Updated the JSONMarshaller based on Mike suggestion.

@justinjdickow
Copy link
Contributor

👍

@mikeburke106
Copy link
Contributor

Merged in commit 43d5fd5

@mikeburke106 mikeburke106 added the REVIEW - accepted The reviewer has approved the PR label Oct 27, 2014
@anildahiya anildahiya deleted the RefactorVectorToList branch October 25, 2016 06:07
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REVIEW - accepted The reviewer has approved the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants