planetscale / database-js

A Fetch API-compatible PlanetScale database driver
https://planetscale.com/docs/tutorials/planetscale-serverless-driver
Apache License 2.0
1.17k stars 35 forks source link

Suggestion: Allow http (and non :433) URLs for testing purposes #135

Closed samlaycock closed 1 year ago

samlaycock commented 1 year ago

Basically this:

import { connect } from '@planetscale/database'

const config = {
  url: 'http://user:password@localhost:3000'
}

const conn = connect(config)

Because....

We're using a home-grown Docker image to allow us to build with @planetscale/database while in offline situations (like the 14 hour flight I was just on)- it's a dumb express server that emulates, as far as we can tell, the current API this package uses. Obviously understanding the API isn't documented, is not really intended for this and is subject to change etc, it would still be good if this package didn't assume the connection URL is an https one when using the config.url value with a valid http/https string.

I've got a fork that implements it (and adds a test), but wanted to float the idea before submitting a PR.

mattrobenolt commented 1 year ago

Tangentially related, I have a little hacked simulator that mirrors the API if you want to more reference this: https://github.com/mattrobenolt/ps-http-sim

I plan to produce something more official, but this works as a HTTP <> mysql proxy.

This sim would still have the exact same issue since it'd be listening over http as well.

I think we really want to prevent any potential insecure mistakes here, but maybe a config option such as allowInsecureConnection: true or something very explicit to allow an http url.

@iheanyi thoughts?

iheanyi commented 1 year ago

@mattrobenolt Let me think more on this, I think you're right though. The reason why we didn't have insecure connections is because the internal systems are always HTTPS and never over raw HTTP. But if there exists a way to use this driver over HTTP, then we should add a configuration setting for it.

mattrobenolt commented 1 year ago

Yeah, the only situations are for something like local dev. It's never really been a priority since we don't provide a way to even run the driver against anything locally, but it seems folks are beating us to it. :)

The long term solution for a local simulator would very likely just listen on plaintext, so it'd likely make sense for us to provide some explicit option to do this.

I'd prefer we don't infer it based on the URL since that could be potentially a typo of http://.. so an explicit config option would be best imo.

samlaycock commented 1 year ago

Appreciate this is an ongoing discussion.. this is what I have - https://github.com/planetscale/database-js/pull/136

Feel free to disregard if you think there's a better way of handling it (it's a very naive approach)

iheanyi commented 1 year ago

This has been fixed as of version v1.11. You can use http in the url configuration setting and it will be respected :) Thanks for the report once again!

samlaycock commented 1 year ago

Awesome. Thanks for the quick work in getting this done!