Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Convert to using Gin and (greatly) simplify #2

Merged
merged 3 commits into from
Jan 7, 2024
Merged

Conversation

justinclift
Copy link
Member

@justinclift justinclift commented Dec 31, 2023

This PR was mostly because I needed a practise project to get the hang of Gin with.

Updating/simplifying this project has been on the wish list for ages anyway, so it's not wasted effort. 😄

@justinclift justinclift force-pushed the gin_v1 branch 3 times, most recently from ffc636d to 9c95767 Compare December 31, 2023 10:49
@justinclift
Copy link
Member Author

@MKleusberg Converting this Gin. So far it's not too bad, though Gin's docs aren't very good. 😦

@justinclift
Copy link
Member Author

@MKleusberg Oh, I've removed the TLSNextProto line when I copied the http.Server piece from the DBHub.io API main.go:

https://github.com/sqlitebrowser/dbhub.io/blob/5f902c78e195839becbe59be041b336383ae4b51/api/main.go#L158

That line seems to be for disabling HTTP/2 support, which I think is to stop the HTTP/2 Rapid Reset attack from working.

Go releases from 1.21.3 onwards aren't susceptible to that attack, so won't affect us.

If that was disabled for some other reason though, then please let me know as we might need to still disable it after all. 😄

@justinclift justinclift changed the title Convert to using Gin, and simplify greatly Convert to using Gin and (greatly) simplify Jan 3, 2024
@justinclift justinclift force-pushed the gin_v1 branch 3 times, most recently from d6488b2 to d5482db Compare January 4, 2024 10:04
@justinclift justinclift marked this pull request as ready for review January 5, 2024 11:30
@justinclift
Copy link
Member Author

@lucydodo @chrisjlocke Not sure if you're interested in reviewing Go code, but just in case you are then I'd welcome your input. 😄

@lucydodo
Copy link
Member

lucydodo commented Jan 6, 2024

Thanks for the opportunity, but I don't know enough about Go to comment. :(

@justinclift
Copy link
Member Author

No worries at all. 😄

@justinclift
Copy link
Member Author

Doesn't looks like anyone's around to review this, so I'm just going to merge it rather than leave it hanging around. 😄

@justinclift justinclift merged commit 195b421 into master Jan 7, 2024
1 check passed
@justinclift justinclift deleted the gin_v1 branch January 7, 2024 02:30
@justinclift
Copy link
Member Author

This is deployed to production now, and seems to be operating ok. 😄

@MKleusberg
Copy link
Member

Just had a first closer look and seems all good so far 😄

Three questions popped up though:

  1. Is the fallback SQLite connection really worth the effort, like would we really merge the data into the actual database afterwards? And what about the connection breaking while this is running, so during startup connecting to Postgres but then having to switch to SQLite later. Or is the SQLite backend maybe just for testing purposes?
  2. Looking at it now I fell like moving all the download files information (file names, dates, etc.) into a database table so this is more data driven.
  3. Would setting the last modified date on the files at startup be an option? So we make sure they have the correct last modified dates and from there on we could use StaticFile or ServeFile? Besides potentially simplifying the code, these function reduce copying by using the sendfile syscall.

And one final thing:
We could/should remove the CORS middleware from this. Besides that one line of code it would also get rid of one dependency. But more importantly I don't think Access-Control-Allow-Origin: * makes a lot of sense here.

@justinclift
Copy link
Member Author

justinclift commented Jan 7, 2024

Thanks for looking over it. 😄

what about the connection breaking while this is running

Haven't tested it, so it could be anything from "silently dropped" to "application aborts and is restarted by systemd, which then records to a database". The 2nd one would be better, but I really have no idea yet what the actual behaviour is. 😄

I was more doing it because we had a case of this happening a while back, where it ran for a substantial amount of time (months?) without recording anything. My intention there is that we can at least capture the data, and then figure out if we want to feed it back into the main database.

Yeah, it's kind of half arsed for now. 😇

2 - Heh. I hadn't though of putting that info into a database instead, but yeah... that would be doable too.

3 - Yeah, that'd probably be workable too. Just didn't think of it. 😄


I've just pushed a commit to master that removes the CORS middleware, and deployed the updated code to the production server.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants