Page MenuHomePhabricator

Writing the HTTP/HTTPS basic request reader
ClosedPublic

Authored by CLOVIS on Jun 25 2018, 2:28 PM.

Details

Summary
  • Added newlines at the end of files
  • Ignore ignorable stuff in modules
  • Creation of the HTTP module
  • First working version for both HTTP & HTTPS
  • Refactoring into other classes
  • Removing empty lines
  • Removing duplication
  • Refactoring to make the code clearer
  • Changed indent_style to space
  • Merge .editorconfig
  • Cleaning & refactoring of the code

Fixes T217

Test Plan

./gradlew test should be successfull with no errors.

Diff Detail

Repository
rLIBWFJVA WildFyre Java Libary
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Info-Screen retitled this revision from Writing the HTTP/HTTPS basic request reader (Fixes T217). to Writing the HTTP/HTTPS basic request reader.Jun 25 2018, 3:17 PM
Info-Screen edited the summary of this revision. (Show Details)
http/src/test/java/net/wildfyre/http/ConnectionTest.java
24

There shouldn't be a need for ftp connections

You need to "git rebase master" this, getting this error: http://prntscr.com/jzbg6d (yes im on master)

.editorconfig
9 ↗(On Diff #593)

Shouldnt this be crlf?

gradle/wrapper/gradle-wrapper.properties
2

Why is this here?
Is this automatic?

http/src/main/java/net/wildfyre/http/Method.java
16

Why only these 3 requests?
You'll more than likely need OPTIONS and DELETE as well

Also just an fyi, when it comes to PUTing or POSTing an image, the request is a multipart/form-data type, rather than application/json

http/src/main/java/net/wildfyre/http/Request.java
50

Read comment above

51

Are you suppose to specify content-length? @Info-Screen is this required?

http/src/test/java/net/wildfyre/http/RequestTest.java
19

For this test to always pass, there must always be a user named libtester, this could be problematic in the future

settings.gradle
4

newline

This revision now requires changes to proceed.Jun 25 2018, 11:50 PM
http/src/test/java/net/wildfyre/http/ConnectionTest.java
24

The point was to verify that it would work for HTTP and HTTPS but not work for other connections, so I tried to connect in FTP and check that an error is thrown at line 29-30

http/src/test/java/net/wildfyre/http/RequestTest.java
19

Yep, a good solution would be that the person handling the web services repo add a script that launches the local dev build with a mock database.

CLOVIS added inline comments.
.editorconfig
9 ↗(On Diff #593)

As you prefer.

gradle/wrapper/gradle-wrapper.properties
2

Every file named in the directory 'gradle' was automatically added (they are scripts so you can launch Gradle from your machine without installing it)

http/src/main/java/net/wildfyre/http/Method.java
16

They will be added in the future, this is only the base blocks for it. Multipart will be added when needed.

http/src/main/java/net/wildfyre/http/Request.java
51

I believe it is required, but I'm not 100% sure.

CLOVIS added inline comments.
.editorconfig
9 ↗(On Diff #593)

Actually I just checked and it's considered better regarding Git to use LF. The convention for cross-platform projects is to use LF and have Git translate them to CRLF when needed. Source on the GIT-SCM website, see "Formatting and whitespace"

  • Added HTTP methods PUT, OPTIONS & DELETE
gradle/wrapper/gradle-wrapper.properties
2

Carry on

http/src/main/java/net/wildfyre/http/Request.java
51

From what I read, it specifies the buffer size, and is helpful in large requests, however, we dont have requests over 100KB.
So ultimately its not needed, but not bad to have either. You would have to calculate this size so that could lead to some overhead between requests

http/src/test/java/net/wildfyre/http/ConnectionTest.java
24

Carry on

CLOVIS added inline comments.
http/src/main/java/net/wildfyre/http/Request.java
51

Based on the fact that I have to convert the full request as a byte array anyway, I don't think asking for its size is the bottleneck here. Finding a way to not convert to an array might be a lot better. Here's a task for that optimization: T223

Multiple Indent issues (pointed one out by inline comment).

And the first commit doesn't have any commit in master as it's parent.

http/build.gradle
17

is this intended to have only 2 space indent instead of 4, like the line above?

settings.gradle
4

empty line here. (So file ends with 2 LF bytes instead of one)

This revision now requires changes to proceed.Jun 29 2018, 2:16 PM
CLOVIS marked 2 inline comments as done.
  • Fixed indent & EOF
http/build.gradle
17

No, it's an indent bug because the .editorconfig was changed from 'tab' to 'space'. It's fixed now.

http/src/main/java/net/wildfyre/http/Method.java
16

Your indentation isnt consistent here

CLOVIS marked an inline comment as done.
  • Fixed problematic indentation.
This revision is now accepted and ready to land.Jul 2 2018, 2:01 PM
  • Merging master
  • Removed javadoc linting
This revision was automatically updated to reflect the committed changes.