RedirectPath #20

kkrull · 2018-05-02T22:08:14Z

Added a playground.RedirectRoute to handle /redirect

Test results

Starting Test System: slim using fitnesse.slim.SlimService.
. 17:06:48 R:0    W:0    I:0    E:0    SuiteSetUp       (HttpTestSuite.SuiteSetUp)      0.002 seconds
F 17:06:48 R:1    W:6    I:0    E:0    BasicAuth        (HttpTestSuite.ResponseTestSuite.BasicAuth)     0.000 seconds
F 17:06:48 R:0    W:4    I:0    E:1    CookieData       (HttpTestSuite.ResponseTestSuite.CookieData)    0.000 seconds
F 17:06:48 R:2    W:8    I:0    E:0    CreateReadUpdateDelete   (HttpTestSuite.ResponseTestSuite.CreateReadUpdateDelete)        0.001 seconds
. 17:06:48 R:6    W:0    I:0    E:0    DirectoryLinks   (HttpTestSuite.ResponseTestSuite.DirectoryLinks)        0.001 seconds
. 17:06:48 R:1    W:0    I:0    E:0    DirectoryListing (HttpTestSuite.ResponseTestSuite.DirectoryListing)      0.000 seconds
. 17:06:48 R:1    W:0    I:0    E:0    FileContents     (HttpTestSuite.ResponseTestSuite.FileContents)  0.000 seconds
. 17:06:48 R:3    W:0    I:0    E:0    FourEightTeen    (HttpTestSuite.ResponseTestSuite.FourEightTeen) 0.001 seconds
. 17:06:48 R:1    W:0    I:0    E:0    FourOhFour       (HttpTestSuite.ResponseTestSuite.FourOhFour)    0.001 seconds
. 17:06:48 R:3    W:0    I:0    E:0    ImageContent     (HttpTestSuite.ResponseTestSuite.ImageContent)  0.000 seconds
. 17:06:48 R:4    W:0    I:0    E:0    MediaTypes       (HttpTestSuite.ResponseTestSuite.MediaTypes)    0.001 seconds
. 17:06:48 R:6    W:0    I:0    E:0    MethodNotAllowed (HttpTestSuite.ResponseTestSuite.MethodNotAllowed)      0.000 seconds
. 17:06:48 R:2    W:0    I:0    E:0    ParameterDecode  (HttpTestSuite.ResponseTestSuite.ParameterDecode)       0.000 seconds
F 17:06:48 R:0    W:11   I:0    E:0    PartialContent   (HttpTestSuite.ResponseTestSuite.PartialContent)        0.000 seconds
F 17:06:48 R:4    W:2    I:0    E:0    PatchWithEtag    (HttpTestSuite.ResponseTestSuite.PatchWithEtag) 0.001 seconds
. 17:06:48 R:2    W:0    I:0    E:0    RedirectPath     (HttpTestSuite.ResponseTestSuite.RedirectPath)  0.001 seconds
. 17:06:48 R:1    W:0    I:0    E:0    SimpleGet        (HttpTestSuite.ResponseTestSuite.SimpleGet)     0.000 seconds
. 17:06:48 R:4    W:0    I:0    E:0    SimpleHead       (HttpTestSuite.ResponseTestSuite.SimpleHead)    0.001 seconds
. 17:06:48 R:6    W:0    I:0    E:0    SimpleOption     (HttpTestSuite.ResponseTestSuite.SimpleOption)  0.001 seconds
F 17:06:48 R:0    W:1    I:0    E:0    SimplePost       (HttpTestSuite.ResponseTestSuite.SimplePost)    0.000 seconds
F 17:06:48 R:0    W:1    I:0    E:0    SimplePut        (HttpTestSuite.ResponseTestSuite.SimplePut)     0.000 seconds
. 17:06:49 R:1    W:0    I:0    E:0    TimeToComplete   (HttpTestSuite.SimultaneousTestSuite.TimeToComplete)    0.002 seconds
. 17:06:49 R:0    W:0    I:0    E:0    SuiteTearDown    (HttpTestSuite.SuiteTearDown)   0.000 seconds
--------
23 Tests,       8 Failures      3.996 seconds.
0
Exit-Code: 7

so it has access to query parameters, etc...

kkrull · 2018-05-02T22:08:57Z

I recommend checking out the previous pull request, first. This pull request is only different by the last couple of commits.

rmulhol · 2018-05-03T19:35:36Z

http/messages.go

+}
+
+type requestMessage struct {
+	method          string


This has been a pattern in some of your other work so far, but I wanted to 👍 the use of private attributes and getters where appropriate

Thanks! I'm hoping to be able to hide a few more types within their respective packages, in the near future.

rmulhol · 2018-05-03T19:39:35Z

http/messages.go

+	}
+}
+
+func NewOptionsMessage(target string) RequestMessage {


Any reason for the naming mismatch between this and the other like methods (target vs path)?

You're right - there are a few places I need to update for the other methods, since just about everything operates on the path portion of the target instead of the whole target.

This case does happen to be an edge case, where OPTIONS * HTTP/1.1 (note the lack of / in the target) is a valid message: https://tools.ietf.org/html/rfc7230#section-5.3.4. That's why there's a different name for this one, although maybe some better naming could further clarify the intent.

rmulhol · 2018-05-03T19:41:14Z

http/messages.go

+
+func NewGetMessage(path string) RequestMessage {
+	return &requestMessage{
+		method: "GET",


I would probably extract these strings representing methods into some type of enum so that you don't have to type out the same string in various places/can get help from the compiler in the case of a fat finger

Good idea - I'm extracting some consts

rmulhol · 2018-05-03T19:42:23Z

http/messages.go

+	return &requestMessage{
+		method: method,
+		target: path,
+		path:   path,


Perhaps I need to be poking around deeper into the codebase, but it's not clear to me why these RequestMessage objects have target and path attributes, given that they seem to always be the same

Agreed that this is causing some confusion, even on my end. I think I got it pretty well cleaned up for GET requests, and I'm planning to address that during the next couple of stories (which deal with POST, PUT, and DELETE) requests.

rmulhol · 2018-05-03T19:43:35Z

http/messages.go

+}
+
+func (message *requestMessage) AddQueryFlag(name string) {
+	message.queryParameters = append(message.queryParameters, QueryParameter{Name: name})


If a QueryParameter without a Value is a QueryFlag, perhaps it deserves its own object?

I like the idea in general, to avoid a de-generate data structure. Let me get back to you on this one, as it would require some refactoring on the test side.

rmulhol · 2018-05-03T20:03:31Z

http/percent.go

+}
+
+func splitAfterHexCode(hexCodePlusUnencoded string) (hexCode string, unencoded string) {
+	return hexCodePlusUnencoded[0:2], hexCodePlusUnencoded[2:]


I think you could use hexCodePlusUnencoded[:2] here

rmulhol · 2018-05-03T20:03:48Z

http/percent.go

+}
+
+func decode(octetCharacters string) byte {
+	asciiCode, _ := strconv.ParseInt(octetCharacters, 16, 8)


what are 16 and 8 representing here?

I'll extract some constants to describe it better.

rmulhol · 2018-05-03T20:04:56Z

http/percent_test.go

+
+	It("returns an error when % is followed by 1 character", func() {
+		_, err := http.PercentDecode("abc%1")
+		Expect(err).To(MatchError("% followed by fewer than 2 characters: abc%1"))


I might consider a custom error you could match against so that you don't need to repeat the exact string formatting

rmulhol · 2018-05-03T20:06:07Z

main/cmd/factory_test.go

@@ -91,7 +91,7 @@ var _ = Describe("InterruptFactory", func() {
 			Expect(typedServer.Routes()).To(ContainElement(BeAssignableToTypeOf(capability.NewRoute())))
 		})

-		XIt("has a playground route for parameter decoding", func() {
+		It("has a playground route for parameter decoding", func() {


rmulhol · 2018-05-03T20:07:54Z

playground/redirect_route.go

+}
+
+func (*GoBackHomeResource) Get(client io.Writer, req http.RequestMessage) {
+	msg.WriteStatusLine(client, 302, "Found")


Status codes and messages are also things I would consider wrapping in an enum

Agreed. I modified msg.WriteStatusLine and to accept pre-defined msg.Status objects. They're not guarded against mutation or re-assignment, but this seems to follow the pattern in the Go standard libraries nonetheless.

...and strip their names of magical powers

kkrull added 30 commits April 26, 2018 15:48

Added missing test

784360c

Moved MakeResourceRequest over to a method on RequestLine

1e6c622

Making room for RequestMessage interface

a013129

Introducing RequestMessage interface

ecf0bce

Routes now depend upon the http.RequestMessage interface

7134734

Using a constructor to make RequestMessages

ccbe9e6

Now only the http package is using the concrete type

1d815ba

Re-enabled the /parameters route

d9841cd

Passing RequestMessage to Resource#Get

bfa2e42

so it has access to query parameters, etc...

Extracted method-specific RequestMessage constructors

314070d

Notes

52ff3a9

Stubbing in a query for query parameters

db3852b

Looking at the path instead of the whole target

04bfb67

Renamed fields in requestMessage

dc57d34

Using RequestMessage#Path instead of #Target

6e43c47

Using path instead of target for capability

1b42015

Path instead of target for fs

efebe64

Path instead of target for http

5794a5a

Path instead of target for playground

9c2b731

Path instead of target for teapot

3912ece

WIP parsing query strings

508eff0

Refactoring

7a7e793

Moving tests over to RequestParser to consolidate parsing responsibility

d2a97c6

Clarified state machine in parseMethodObject

6b391ad

Moved query string parsing over to LineRequestParser

3ce0dc4

Renamed file

4bd6368

Placeholder tests

2e60a6d

Decoder -> Reporter

bf3ee18

Listing query parameters

328eabe

format

e92a624

kkrull added 6 commits April 30, 2018 22:02

Cleaned up unused mock

7fa876b

percent decoding

50cb4c5

No need to convert to []byte for percent decoding

54d575d

Making sure decoding handles errors

15362d6

Added a route to /redirect

05a1dca

GoHomeResource for redirect

1a73779

rmulhol approved these changes May 3, 2018

View reviewed changes

kkrull mentioned this pull request May 3, 2018

Parameter decoding #19

Closed

kkrull added 7 commits May 3, 2018 15:59

Addressed review findings

86168d8

Introduced constants to DRY HTTP methods...

9a9f6d8

...and strip their names of magical powers

Extracted constants for some magic numbers

f3ccc2c

Multiple returns to guard against missing nil checks

71748e1

Extracted a custom error type for malformed octets

74220e1

DRY HTTP status codes/reasons

a3268e7

Standardized one more IANA HTTP Method

a51b908

kkrull merged commit a4a2b9a into master May 7, 2018

kkrull deleted the redirect branch May 7, 2018 01:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RedirectPath #20

RedirectPath #20

kkrull commented May 2, 2018

kkrull commented May 2, 2018

rmulhol May 3, 2018

kkrull May 3, 2018

rmulhol May 3, 2018

kkrull May 3, 2018

rmulhol May 3, 2018

kkrull May 4, 2018

rmulhol May 3, 2018

kkrull May 4, 2018

rmulhol May 3, 2018

kkrull May 4, 2018

rmulhol May 3, 2018

kkrull May 4, 2018

rmulhol May 3, 2018

kkrull May 4, 2018

rmulhol May 3, 2018

kkrull May 6, 2018

rmulhol May 3, 2018

kkrull May 6, 2018

rmulhol May 3, 2018

kkrull May 7, 2018

RedirectPath #20

RedirectPath #20

Conversation

kkrull commented May 2, 2018

Test results

kkrull commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment