Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add some examples or more documentation? #187

Open
ragnese opened this issue Apr 7, 2022 · 2 comments
Open

Add some examples or more documentation? #187

ragnese opened this issue Apr 7, 2022 · 2 comments

Comments

@ragnese
Copy link

ragnese commented Apr 7, 2022

This is very tangentially related to #186 in that I've never been totally sure of the recommended way(s) to work with mysql_async's API when it comes to abstracting and writing reusable functions.

The example in the main docs is very good for a first impression of the library, but it might be helpful to document recommended ways to work with the library for more involved code/modules. I will list a few questions/scenarios here. I'm not asking these questions to be answered here (though, that wouldn't hurt, as I'm still not 100% sure on some of them, myself). The intention is that I think it would be helpful for these questions to be answered by the documentation.

  • I have a function called something like update_user that does a SELECT FOR UPDATE query, following by some logic, and then the actual UPDATE user SET [...]. Obviously, these operations are required to happen in a transaction and not just any connection. Is it recommended/appropriate/optimal for the function to have a parameter tx: &mut Transaction<'_> or is there some downside to using the concrete type and we should prefer to accept a &mut Queryable instead, even though the logic wouldn't be fully correct for a Conn argument?
  • Why is there an error for calling start_transaction on a Conn that already has a transaction going? AFAIK, MySQL doesn't have the concept of nested transactions, but it's also not an error to execute a START TRANSACTION multiple times before executing a COMMIT or ROLLBACK. It's fine that this library made that choice, but some "in your face" documentation should warn users about this because it does differ from native MySQL behavior. (Again, AFAIK- I could be mistaken)
  • In the docs example, there's a let loaded_payments = ... query. Is the with(()) call necessary?
  • Is the Query trait version of the API somehow "preferred" or "recommended" over calling the Queryable methods?
  • The Queryable methods seem to differentiate between prepared statements and "plain" queries with prefixes in the method names ("query" vs. "exec"), but the Query methods do not (it seems like they all will attempt to prepare statements; is that right?). Is there a reason for that?
  • Maybe just some more documentation around prepared statements in general would be good. Like, how and when you should explicitly prepare statements, or when the library will try to do it for you, etc.

Those are just things I remembering asking myself while using this excellent library. Most of the time, I ended up needing to dig through the code to learn the answers, and I feel like it would help new users a ton to have them answered in the docs somewhere or in a Cargo examples/ directory or something.

Thanks again, and cheers!

@blackbeam
Copy link
Owner

Hi. Yeah, I need to find time to extent the readme.

Regarding the questions (in order):

  1. I found it practical to accept Queryable and to manage transaction lifetime in the outer code. Actual case: I wrote a backend so that it's impossible to acquire a raw Conn – handler receives a context instance with a fresh transaction in it and its lifetime is managed by the server runtime.
  2. You should note that some statements causes an implicit commit (START TRANSACTION is one of them), so this behavior is chosen as less error prone.
  3. The reason here is more subtle – the with call creates QueryWithParams that is executed using the binary protocol (i.e. as a prepared statement) and the produced result is also binary (not textual). First call to this may be slower (if number of rows is small enough), but subsequent calls will be faster, because statement is already in the cache and driver won't have to parse textual data. I believe this is slower in the case of this concrete example, but faster in general, that's why it is here.
  4. It's not always possible to use Query trait because of it's consuming API, but I like that it's abstracted over the protocol so that it's API is nice and small. It also allows you to run directly on a Pool and to conveniently create 'static futures.
  5. Differentiation of a protocol for the Query trait is performed via the Query::Protocol associated type (it uses TextProtocol for strings and BinaryProtocol for QueryWithParams)
  6. First of all you should note that it's not always possible to prepare a statement (see here). Regarding pros and cons:
    • - if statement is no in the cache, than one additional prepare request is performed (worth noting that statement's scope is the connection that was used to prepare it)
    • + prepared statements emits a binary result (this does not matter if all columns are textual)
    • + prepared statements protects against SQL injections (it's wise to use them for all parametrized queries)

@ragnese
Copy link
Author

ragnese commented Apr 15, 2022

Thank you for the thorough reply.

  1. That seems reasonable.
  2. Yes, the "native" behavior of how MySQL handles "nested" transactions is definitely error-prone, and I think that your choice is actually preferable to MySQL's behavior. I just think it's important to document that this library actually chooses to differ from the underlying "standard" behavior.
  3. Oh! I didn't realize that the Query API allowed us to control whether we're working with prep'd statements or not. That's good to know!
  4. That's true. Very nice.
  5. See Fix parse_named_params() when there's special chrs in the query #3 :) Thank you.
  6. I do know about prepared statements, etc, with MySQL. What I meant by my suggestion was that maybe it would be beneficial to have a whole section of documentation for how this library works with prepared statements vs text protocol queries. In my opinion, it would be written to target an audience that already knows/understands how MySQL prepared statements work, but isn't sure how to use your APIs to control whether or not something is prepared, etc.

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

No branches or pull requests

2 participants