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

[TIMOB-25241] Android: Do not throw exception on HTTP 400 codes #9411

Merged
merged 6 commits into from Nov 15, 2017

Conversation

garymathews
Copy link
Contributor

@garymathews garymathews commented Sep 7, 2017

  • Prevent an exception being thrown on 400 response codes to match iOS behaviour
TEST CASE
var request = Ti.Network.createHTTPClient({
	onload: function() {
		console.log('onload: ');
		console.log('readyState: ' + getReadyStateNameFrom(request));
	},
	onerror: function(e) {
		console.log('onerror: ', e);
		console.log('readyState: ' + getReadyStateNameFrom(request));
	}
});
request.open('GET', 'http://www.appcelerator.com/test.js');
request.send();

function getReadyStateNameFrom(httpClient) {
	switch (httpClient.readyState) {
		case httpClient.UNSENT: return 'UNSENT';
		case httpClient.OPENED: return 'OPENED';
		case httpClient.HEADERS_RECEIVED: return 'HEADERS_RECEIVED';
		case httpClient.LOADING: return 'LOADING';
		case httpClient.DONE: return 'DONE';
	}
	return 'UNKNOWN';
}

JIRA Ticket

Copy link
Contributor

@jquick-axway jquick-axway left a comment

Choose a reason for hiding this comment

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

@garymathews, HTTP responses having status codes >= 400 are supposed to invoke the onerror() listener. This is how we document it (link below) and how it works on iOS (see iOS log output in JIRA case).
http://docs.appcelerator.com/platform/latest/#!/api/Titanium.Network.HTTPClient-property-onerror

So, we may want to do the following in the Java setReadyState() method...

public void setReadyState(int readyState)
{
	Log.d(TAG, "Setting ready state to " + readyState, Log.DEBUG_MODE);
	this.readyState = readyState;
	KrollDict data = new KrollDict();
	data.put("readyState", Integer.valueOf(readyState));
	dispatchCallback(TiC.PROPERTY_ONREADYSTATECHANGE, data);

	if (readyState == READY_STATE_DONE) {
		KrollDict data1 = new KrollDict();
		if (getStatus() >= 400) {
			if (getStatusText == null) {
				setStatusText("");
			}
			data1.putCodeAndMessage(getStatus(), getStatus() + " : " + getStatusText());
			dispatchCallback(TiC.PROPERTY_ONERROR, data1);
		} else {
			data1.putCodeAndMessage(TiC.ERROR_CODE_NO_ERROR, null);
			dispatchCallback(TiC.PROPERTY_ONLOAD, data1);
		}
	}
}

Personally, I would prefer onload to be invoked for HTTP errors responses too (like you're doing it) and only invoke onerror for socket errors, timeouts, and SSL errors... but that's not how it's documented/designed.

Copy link
Contributor

@jquick-axway jquick-axway left a comment

Choose a reason for hiding this comment

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

CR: Pass

@ssjsamir ssjsamir self-requested a review October 9, 2017 14:35
@build
Copy link
Contributor

build commented Oct 9, 2017

Fails
🚫

Tests have failed, see below for more information.

Warnings
⚠️

🔬 There are library changes, but no changes to the unit tests. That's OK as long as you're refactoring existing code

Tests:

Classname Name Time Error
ios.Titanium.UI.Window window_navigation 5.075 file:///Users/build/Libra

Generated by 🚫 dangerJS

@mukherjee2 mukherjee2 self-requested a review November 15, 2017 01:31
Copy link
Contributor

@mukherjee2 mukherjee2 left a comment

Choose a reason for hiding this comment

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

Passed FR with this environment:
Node Version: 6.11.5
NPM Version: 3.10.10
Mac OS: 10.13
Appc CLI: 7.0.0-master.8
Appc CLI NPM: 4.2.11-2
Titanium SDK version: 7.0.0 locally built
Appcelerator Studio vers 4.10.0
Android 7.1.2

@eric34 eric34 merged commit fcec35b into tidev:master Nov 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants