Catch exceptions thrown when page number is not an integer

[danielcompton]

At https://github.com/clojars/clojars-web/blob/33/src/clojars/web.clj#L52, there is no exception handling for when the parameters can't be parsed as an integer. This should be handled and return an error page (500?) to the user.

A test case is: https://clojars.org/search?q=3&page=%26%2339%3b%2cnetsparker(0x000C93)%2c%26%2339%3b

or for your local app: http://localhost:8080/search?q=3&page=%26%2339%3b%2cnetsparker(0x000C93)%2c%26%2339%3b

[tomjkidd]

Was able to verify the issue, and there are some questions for me (or anyone else) in order to not waste effort on a solution without agreement on what to do.

Technically, the request contains invalid query parameters and a 400 Bad Request would be the strict http response code.

Should an exception with this status code be created (and then handled) in order to communicate the problem to the user? How much and what details are appropriate to include on the error page?

[danielcompton]

400 sounds good. I think an error coming back saying that page must be an integer is a good amount of disclosure. It'll also need to be handled in the JSON search results too.

[danielcompton]

See #472 for details on how to throw without reporting to Yeller.

[tomjkidd]

I'm attempting this on windows, and the path to running locally is mighty perilous.

I have more detailed information, but the main issues are:

1. The first few steps of (migrate) terminate due to the jars table not existing, I had to run the queries from https://github.com/clojars/clojars-web/blob/master/resources/queries/clojars.sql using SQLite Manager for firefox
2. The current implementation uses / in at least one path regex, which prevented migrate from running correctly. I changed this on my local copy and tried to move forward now that (go) works in the repl.
3. I tried to access http://localhost:8080/ and get an error
   No implementation of method: :-report-error of protocol: #'clojars.errors/ErrorReporter found for class: clojars.errors.StdOutReporter
4. I tried to access http://localhost:8080/search?q=lein and get an error
   Could not search; please check your query syntax.

Thought I might just mention what I've done so far in case I am missing something.

As for a solution to the actual problem, I was thinking about modifying the error-page-response function to take an optional parameter hash map that would allow you to specify the title, error message, and status while reusing most of the existing function and defaulting to the values that are already there. This would make it so that if we can identify the type of error in code, the options can be filled in and passed with the throw, and then the same wrapper will catch and apply them. I'd have to look more at the JSON code, but it seems like a similar story.

[danielcompton]

Hi Tom, thanks heaps for your detailed writeup of your (frustrating) experience. That's really useful as I don't run Clojars on Windows so I miss these kind of cross platform issues.

1. The first few steps of (migrate) terminate due to the jars table not existing,

I think what is happening is that an exception is being thrown at https://github.com/clojars/clojars-web/blob/master/src/clojars/db/migrate.clj#L12, but it is being silently swallowed.

I've pushed eb7b0ba, can you take another pass and see if that fixes that problem?

1. regex

Try using (File/separator) in place of the /?

1. error reporter

I've noticed this too lately, try running (go) again, it should work after that?

1. search

Have you set up the search repo for this?

I'm out this weekend so I don't have time to dig into this too much, but I'll definitely take a look next week and try setting up Clojars on my Windows laptop to see if I can fix some of these bugs.

Thanks for your detailed notes, they are really really helpful!

[tomjkidd]

Thanks for replying so quickly.

1. Running (migrate) with latest still gives me that same error

user=> (migrate)
Running migration: initial-schema
Running migration: add-promoted-field

BatchUpdateException batch entry 0: [SQLITE_ERROR] SQL error or missing database (no such table: jars)  org.sqlite.jdbc3.JDBC3Statement.executeBatch (JDBC3Statement.java:210)

1. I modify https://github.com/clojars/clojars-web/blob/master/src/clojars/dev/setup.clj#L73 locally like this to get it working. This is because replacing it in the regex makes it harder to understand the intent, and because some of the config uses / for specifying directories, it might mix and match with weird results.

[_ group-path artifact-id] (re-find group-artifact-pattern (str/replace (.getPath parent) (java.io.File/separator) "/"))

1. work after (go) (go)
2. Same as 3
   Thanks again for responding so fast.

NOTE: Looks like markdown doesn't respect numbering for the items, they are in order 1-4

[tomjkidd]

Alright, so one final clarification: The verification of the :page param is done early in the /search handler. The name validated-params seems to mean that validation was planned to happen here, however the json/html branch happens in the search function which is called later, which makes it awkward with the current implementation to handle both cases. (Also, page isn't currently used for json-search.)

Is the desire to push page verification into both the json-search and html-search functions so they can each separately handle it?
Is the intent to start using page in the json-search?

I do have a working implementation similar to what I described for the html handler, but wanted to get context for the desire overall.

[tomjkidd]

Alright, I made a fork and put in changes that solve the issue for both html and json versions.

https://github.com/tomjkidd/clojars-web

I am hesitant to make a pull request because the tests don't pass on windows (I don't believe this has anything to do with code I wrote), but could you review the changes just as a sanity check that this is what you want?

Note there are 2 commits at the top. The first is trivial to get the regex working on windows, the second is the actual handling of a bad page parameter.

[danielcompton]

Making a PR is probably the easiest way for us to review and comment. Sorry I haven't gotten back to you about the Windows specific issues, been dealing with a young family :)

[danielcompton]

And thanks!

[tomjkidd]

No worries. I've done the pull request. Thank you