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

HTTP.request function upgrade! #515

Closed
wants to merge 15 commits into from
Closed

HTTP.request function upgrade! #515

wants to merge 15 commits into from

Conversation

@JasonTheKitten
Copy link

@JasonTheKitten JasonTheKitten commented Feb 1, 2018

I think that this time it will pass the travis-ci
Edit: Yay, it did. Time before this it did not

Added important HTTP features used by many real-world applications!
Compatibility is retained with 99.9% of programs!
Interact with servers that require non-get/post methods!

Updates include getting the text that goes with the response code from the server instead of using a lookup table that may not match the server's, the ability to use methods other than post and get, and also the ability to enable or disable automatic page redirects.

I thought about adding a way to get the encryption certificate, but decided against it.
Also, due to limitations of httpurlconnections, there is a whitelist of usable methods. This can probably be changed by writing a script to communicate directly with a socket, but under the same name and with the same methods (so we don't have to change all of our other code), but I decided to let whoever else do that, or maybe I'll do that later, but right now I'm not too worried about it.

@Override
public void shutdown( )
Copy link
Contributor

@SquidDev SquidDev Feb 1, 2018

Choose a reason for hiding this comment

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

I don't like to nitpick, but it's probably best to keep whitespace changes to a minimum. I know indent styles are rather a personal matter, but it's best to be consistent :). It also makes it easier to review the PR and see what functionality has changed, rather than what's just cosmetic.

Copy link
Author

@JasonTheKitten JasonTheKitten Feb 1, 2018

Choose a reason for hiding this comment

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

BTW, it really annoys me that the {'s seem to be on a separate line than the code controlling the block, but I try to follow that formatting anyways. Don't know why.

Copy link
Contributor

@SquidDev SquidDev Feb 1, 2018

Choose a reason for hiding this comment

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

Yeah, there's lots of things I'm not a fan of as far as CC's formatting goes, but you have to roll with it :).

}

//Get mode
String DMODE = "GET";
Copy link
Contributor

@SquidDev SquidDev Feb 1, 2018

Choose a reason for hiding this comment

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

Also minor stylistic choice, but probably shouldn't be UPPER_CASE - that's normally reserved for constants.

@@ -48,37 +48,47 @@ public static URL checkURL( String urlString ) throws HTTPRequestException
return url;
}

public HTTPRequest( String url, final String postText, final Map<String, String> headers, boolean binary ) throws HTTPRequestException
Copy link
Contributor

@SquidDev SquidDev Feb 1, 2018

Choose a reason for hiding this comment

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

This whole file shouldn't exist - I'm pretty sure it's me mucking up a rebase at some point. It's probably best to not edit it at all and someone can delete it in the future (or just delete it now).

Copy link
Author

@JasonTheKitten JasonTheKitten Feb 1, 2018

Choose a reason for hiding this comment

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

Thing doesn't seem to work without it though...

Copy link
Contributor

@SquidDev SquidDev Feb 1, 2018

Choose a reason for hiding this comment

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

Strange. From what I can tell, all references are pointing towards apis.http.HTTPRequest instead of apis.HTTPRequest. What error do you get?

Copy link
Author

@JasonTheKitten JasonTheKitten Feb 2, 2018

Choose a reason for hiding this comment

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

I might be wrong, I'll have to check

Copy link
Author

@JasonTheKitten JasonTheKitten Feb 2, 2018

Choose a reason for hiding this comment

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

Ok, I was wrong. I was thinking of a different file...

case 2:
{
// getResponseCodeText
return new Object[] { responseCodeText };
Copy link
Contributor

@SquidDev SquidDev Feb 1, 2018

Choose a reason for hiding this comment

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

Instead of adding a separate method, I wonder if we could just return both in getResponseCode? Might be a slightly cleaner API.

Copy link
Author

@JasonTheKitten JasonTheKitten Feb 1, 2018

Choose a reason for hiding this comment

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

Maybe, can I get Lupus's opinion first though?

@SquidDev
Copy link
Contributor

@SquidDev SquidDev commented Feb 1, 2018

Aside from my above comments it looks good.

As so far as additional methods go, I've been looking into using Netty's HTTP client instead, as it imposes less restrictions (and also drops the need to have separate threads for each request). One'd probably want to wait until #395 has been reviewed, but it might also be worth looking into.

@JasonTheKitten
Copy link
Author

@JasonTheKitten JasonTheKitten commented Feb 1, 2018

Their never seems to be a reply option to lupus's comments...

Wrong PR?

Branch copy error, I'm worried that deleting it might delete my other branch?

@SquidDev
Copy link
Contributor

@SquidDev SquidDev commented Feb 1, 2018

Branch copy error, I'm worried that deleting it might delete my other branch?

It shouldn't - if you're rebasing, it'll just delete those commits on this particular branch.

@JasonTheKitten
Copy link
Author

@JasonTheKitten JasonTheKitten commented Feb 1, 2018

Ok, got rid of that :)

@SquidDev
Copy link
Contributor

@SquidDev SquidDev commented Apr 4, 2018

Someone added a request similar to this one on the CC:T issue tracker, so I thought I'd have another look at this and raise a few suggestions. One worry I have is that http.request (and other http methods) are ending up like read, with far too many arguments to keep track of.

One possible solution would be to determine if the first argument is a table (falling back to the current system if not) and extract our options from there. This could use a scheme similar to luasocket.http.request:

http.request {
  url = string,
  [method = string,]
  [headers = header-table,]
  [body = string],
  [redirect = boolean,]
  [binary = boolean,]
}

This means we can add as many arguments and options as we like (well, within reason) whilst still keeping calls readable and maintaining backwards compatibility. Obviously this does make argument parsing more complex (though one could create some helpers for table parsing on the Java side), but I think it's worth it.

EveryOS EveryPlatform added 2 commits Apr 9, 2018
@JasonTheKitten JasonTheKitten deleted the master-http branch Apr 10, 2018
@JasonTheKitten JasonTheKitten restored the master-http branch Apr 10, 2018
@JasonTheKitten
Copy link
Author

@JasonTheKitten JasonTheKitten commented Apr 10, 2018

I had accidentally deleted this branch...

SquidDev added a commit to cc-tweaked/CC-Tweaked that referenced this issue May 15, 2018
This implements an argument format similar to LuaReqeust, as described
in dan200/ComputerCraft#515. The Lua argument checking code is a little
verbose and repetitive, but I'm not sure how to avoid that - we should
look into improving it in the future.

Closes #21
@thatcraniumguy
Copy link

@thatcraniumguy thatcraniumguy commented May 23, 2018

Can one of the admins verify this patch?

@SquidDev SquidDev mentioned this pull request Jan 10, 2019
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants