gddo-server: do not 404 when URL contains unknown query parameters

[m90]

Currently, the server sends a 404 when the URL contains query parameters unknown to the application. Instead, ignore the parameters. Fixes #500

[gopherbot]

This PR (HEAD: d7bd9a4fe7f3ef5d139767bfd7031b0ffa2bfcf0) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/#/c/gddo/+/103400 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off) See the Wiki page for more info

[gopherbot]

Message from Gobot Gobot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps: Within the next week or so, a maintainer will review your change and provide feedback. See https://golang.org/doc/contribute.html#review for more info and tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be surprising to people new to the project. The careful, iterative review process is our way of helping mentor contributors and ensuring that their contributions have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which little code gets reviewed or merged. If a reviewer responds with a comment like R=go1.11, it means that this CL will be reviewed as part of the next development cycle. See https://golang.org/s/release for more details.

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/103400. After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Dmitri Shuralyov:

Patch Set 1: Code-Review+1

The general approach of the fix makes sense to me. You're changing this case to trigger not just when len(req.Form) == 0, but in all situations where the other cases within the switch statement are not satisfied.

I have a question about it: what's the reason you decided to move the case from top to bottom of the switch (instead of leaving it in its original position)?

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/103400. After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Frederik Ring:

Patch Set 1:

That's a good question. It's not necessarily needed, but I found the switch statement very hard to read when the default case is at the very top (which forces you to jump up and down when reading it). If you would prefer to have less diff noise instead I would also be fine with changing just the case and make this a single line change.

That being said there would be another option to solving this: Right now the func ends with a return statement that is there purely for the compiler as the switch statement will always run one of its cases and all of its cases return. So in theory, the now default case could also be moved out of the switch and become the final part of the func.

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/103400. After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Frederik Ring:

Patch Set 1:

Scrap the second part, some of the blocks do have break statements so it's not an option. Sorry for the confusion.

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/103400. After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Dmitri Shuralyov:

Patch Set 1:

That's a good question. It's not necessarily needed, but I found the switch statement very hard to read when the default case is at the very top (which forces you to jump up and down when reading it). If you would prefer to have less diff noise instead I would also be fine with changing just the case and make this a single line change.

Thanks for providing the rationale. I'm in agreement. Current version is good, no need to change it.

Scrap the second part, some of the blocks do have break statements so it's not an option. Sorry for the confusion.

Yeah, I checked earlier whether the return after the switch would become dead code after the change to have a default case, but as you point out, it still runs when one of the breaks are hit.

Personally, I wouldn't remind refactoring the code to replace the breaks with return &httpError{status: http.StatusNotFound} and removing the return statement at the end (after the switch). I think that'd be more readable. But this is optional.

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/103400. After addressing review feedback, remember to publish your drafts!

[gopherbot]

This PR (HEAD: f2754463e02fbae9a07ff625669ab1cdc75aaa2a) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/#/c/gddo/+/103400 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off) See the Wiki page for more info

[gopherbot]

Message from Frederik Ring:

Patch Set 2:

Personally, I wouldn't remind refactoring the code to replace the breaks with return &httpError{status: http.StatusNotFound} and removing the return statement at the end (after the switch). I think that'd be more readable. But this is optional.

I also think that's a good idea, so I pushed the changes.

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/103400. After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Dmitri Shuralyov:

Patch Set 2: Code-Review+2

Thanks. This looks good to me (assuming tests pass and build succeeds).

I'll leave merging this CL to someone who has access to deploy the new version (I don't).

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/103400. After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Tuo Shan:

Patch Set 2: Code-Review+2

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/103400. After addressing review feedback, remember to publish your drafts!

[gopherbot]

This PR is being closed because golang.org/cl/103400 has been merged.