-
Notifications
You must be signed in to change notification settings - Fork 28
Allow safe usage of mysql-native #214
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
Conversation
Awesome work, and good catch on how You make a good point about the build system. As it's grown into what I needed, it has indeed gotten a somewhat impenetrable. At the very least, I should document an overview of how it works, or think about if there's any ways I can simplify it without sacrificing the reasons it's become the way it is. I can adjust the phobos-based test scripts to work with taggedalgebraic. I'll probably submit it as a PR to this PR (I think github can do that, iirc...) |
examples/homePage/example.d
Outdated
auto id = row[0]; | ||
auto name = row[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a test for some of the README's example code, so I'd prefer it show the actual type rather than auto
...Just so new users browsing the readme can see exactly what type is expected here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, but I'll hold off until you make a decision on whether we should be doing MySQLVal by default and not Variant.
I can't take credit! The unittests found it, and in fact on the example.d changes that you noted (where I changed the This is the biggest decision to make here. Because I'd have to rework a bunch of the API if we want to be fully backwards compatible in this instance. |
Oh, BTW, this goes without saying, but I should still say it. I think this change would REQUIRE a major bump to 3.0.0. At least that should keep projects that depend on the Variant version from fetching this one. One other possible way to provide a transition is to release version 2.4.0 which uses MySQLVal internally, provides the safe alternatives, and then release 3.0.0 which switches to the safe ones by default. I think I can figure out how to provide all the API calls to allow for code to build with both the old and the new through use of |
Definitely. Except it'll be 4.0.0, because I'm just about to (finally) release the version that makes the switch from "vibe-d:core" to "vibe-core" (and all those related fixes) as 3.0.0 today.
If you're good with that, then that sounds fantastic to me. (Although, again, that'll be 3.1.0 and 4.0.0, rather than 2.4.0 and 3.0.0) Also, since you're the person with the deepest understanding of these changes, would you mind also writing up a small doc that lists and guides people through the changes they'll face in 3.1.0 and 4.0.0? (Maybe a |
source/mysql/commands.d
Outdated
assert(cn.exec(prepareSQL, [Variant(3), Variant("cc")]) == 1); | ||
assert(cn.exec(prepareSQL, [MySQLVal(3), MySQLVal("cc")]) == 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just thinking, maybe we should keep both Variant
and MySQLVal
versions of all the unittests for as long as we're still aiming to not break Variant-based user code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, good point.
source/mysql/commands.d
Outdated
assert(value.get.type != typeid(typeof(null))); | ||
assert(value.get.kind != MySQLVal.Kind.Null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things like this would be good to include in a migration document for people. Like, "The Variant-way vs The MySQLVal-way" and what will/won't be automagically handled by your compatibility tools.
source/mysql/result.d
Outdated
import mysql.types; | ||
public import mysql.types; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, but my thinking is that users who don't opt to just import everything via import mysql;
probably wouldn't be real big on public imports like this and would be perfectly happy importing result
and types
separately.
Or was there another reason for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the main reason I did this is because the MySQLVal type does not operate the same as Variant. With Variant, even if you don't import std.variant, you can use a lot of it it. Not the same with TaggedAlgebraic
and its various UFCS methods. Only assignment, indexing, operators, and unionized methods are exposed via the type itself.
For example, conn.queryValue("select val from tab").get!long
would fail to compile without importing mysql.types or taggedalgebraic (which defines a different get
).
Thinking about this now, maybe there's a way to wrap the TaggedAlgebraic into a type, and put all the "UFCS" methods in there (and expose the TA via alias this). I can try this out if you like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, ok, I see. If you did want to do that, I'd certainly have no objection (and it might make for cleaner documentation anyway), but I wouldn't say it's necessary.
OK
OK, I can work on this. |
ping @Abscissa I had a great idea today -- I split/duplicated command.d into 2 modules, a safe one and an unsafe one. If you just import I still need to examine the MySQLPool, I may need a dual-module for that as well, since the callback was not originally marked as safe. I also would like to add a I also took out all the variant deprecation messages. I figure we can do one release of 3.1.0 without deprecations, then do 3.1.1 with deprecations, and then 4.0.0 with the defaults switched, and then 4.x.x with everything unsafe removed. In any case, still passes tests, and I'm still waiting on a way to test Phobos sockets with this. Note that everything that's not public facing is using the safe version. Let me know if you see any user-callable stuff that is shoehorned into safe. |
More on this idea: I'm going to remove the version identifier, and just go with 2 different packages, mysql.safe and mysql.unsafe. Then mysql.package will public-import mysql.unsafe in 3.1.0, and mysql.safe in 4.0.0. If you want to select a specific interface, you import specifically the safe or unsafe package. It should work across the 2 versions. In 4.1.0 we can deprecate the unsafe versions, and remove them in a later 4.x version. I'm also going to write the document you requested on transitioning. Sound good? |
IIRC, as per semver, either deprecating and/or removing the unsafe versions would mean another major version bump (to 5.x) rather than 4.x, which I'm fine with. So I'll have to double-check that. Aside from that minor housekeeping detail, it all sounds good to me! |
One snag I just ran into -- I'll work on with the assumption marking it final is OK (and we will have to bump major rev to do this). |
Nevermind, I got it to work without making them non-virtual. |
I didn't write the mysqln pool class (maybe @s-ludwig did?), but I'm not aware of any intended subclassing use-case. As far as I'm aware, they're only virtual simply because that's D's default. Maybe Sonke can advise?
|
@Abscissa I have the pretty much final structure done here. Everything succeeds with unittests using vibe mode (I still can't run the phobos tests). Don't do a full review yet. I still have some tasks left:
|
FYI, I tested this against my project, and it builds out of the box without any changes :) A good sign. Still haven't tried modifying my project to use the safe API. |
@Abscissa I'm really struggling to deal with this doc build process. Using 2.090, it can't build your version of ddox. I tried all the way down to 2.083.0
|
I'll take a closer look as soon as I can, but off the top my my head, you should be using the included build docs script, not dub. 'dub docs' and such won't work right. I know I did a very-much-needed update of the ddox version it was based on somewhat recently, but it's possible that might've been for one of my other projects. I'll check.
I needed to hack in some features and bug workaround, and needed the changes right away. There should be a readme note about it in the ddox subdir.
That's not unreasonable as a stop-gap, but either way, if there's a problem with people building it, that still needs to be fixed anyway. |
I saw that. And it's very possible that my branch was started before you did that. So maybe a rebase would help. I'm in the middle of adding all the documentation. When I'm done, I will check out master and see if it builds for me. |
Could these be pushed back to ddox directly? I saw the note, but assumed it was really old. |
In thory, sure, as long as the ddox folk are good with it, then yea that would work, especially now that ddox has removed its formerly-problematic build-time reliance on openssl (working around that was one of the other hacks I had in there, but I was recently able to remove it). This fork was only a "just get it working" thing. That said though, the changes I needed to make to the diet templates probably aren't going to be applicable to most ddox users. So ddox would need to unbreak the broken ability for users to override the built-in templates. (I filled an issue for it. Dont have a link offhand, tho - working from a phone atm, so web-site-ing around is awkward...) |
281ca85
to
f526946
Compare
@Abscissa take a look at the migration document. I'm satisfied with it, and the API/code. One final update is to make sure we test both the unsafe and safe API in the unittests. This shouldn't take me too long, but I want to first see if I can fix the documentation build. With any luck, I can have this ready for merge/review by the end of this weekend. |
MySQLVal and related functions caught by full test runs.
Ok @Abscissa I'm pretty much done with this. I rewrote nearly all the unit tests to run with both the safe and unsafe API. It was a good test to see what is different between the two APIs, and as I said above, it caught a major issue I didn't notice before. I settled on an approach where I enclosed all the existing unittest code into a nested template function, which accepts a template parameter In most cases, that's all I had to do, and the thing worked exactly as before, which is a great sign. In some cases, I switched some code up based on the I will note that the Phobos socket tests still don't work (-m=phobos), as they don't include the TaggedAlgebraic module in the rdmd command. I don't know what you want to do there. Let me know. |
BTW, before this is merged, I'm thinking of hand-rebasing this to make the commits cleaner. |
Ping @Abscissa almost 3 months since I heard from you on this. |
Sorry for the silence. I've been following your status updates on this, and very much appreciate the herculean effort you've been putting into it.
|
Thanks for getting back to me. Let me know if there's any way you want me to reorganize the commits to make this easier to review or any questions you have. It's way huger than I thought it would be at the start. |
So far, just going through the overall "Files Changed" view is proving straightforward enough and I haven't needed to drill down into individual commits. And for the moved/copied/split/renamed files that git/github have trouble understanding, I have a good comparison tool that will probably handle those fine as well, so reorganizing any commits probably won't be necessary. |
Still working on this, just felt it appropriate to give a status update so it doesn't look like I've TOTALLY disappeared...(Again, thanks for your patience with me on this...) I got roughly half-way through my github code-review on this and then, a couple weeks ago, my main job picked back up after its extended COVID-19 slump, temporarily pulling me back away again. But, FWIW, I haven't come across any remotely major issues so far - most of my notes at this point are pretty much trivial doc-related nitpicks (ie, "English": The only thing D compilers don't statically validate) and a few small questions for my own benefit. The longer this takes, the more it nags at me for my own time management, so rest assured I'm committed to getting through the rest of this ASAP, and getting it released and finally out the door. Thanks to both you especially and also other contributors, this lib has already improved in ways I never would have imagined since I assumed maintainer duties, and yet there's still plenty more I've been dreaming of for this lib. This is the current big "next step" towards these goals, and a very important one... |
Just a gentle monthly ping @Abscissa, in case you need one. No worries if you are too busy still. |
Thanks, pings are always appreciated (and sometimes all too necessary...). I had REALLY wanted to reply to this with an "I'm back on it right now", but obviously that hasn't quite happened. This has proven to be quite an odd year for me, personally, even aside from COVID-19, and regretfully, I feel like I've been a neglectful project manager for this lately. But at the same time, I'm not comfortable pushing a big, albeit incredibly important, changeset through without at least a second set of experienced eyes on every line... So if it's ok with you, I think what I'd like to do is tentatively push this as an Alpha release in the interim. From what I've seen so far of both this and your prior contributions, I'm not expecting any major issues or changes, and I hate to delay this any further from anyone who may benefit from it, simply because of formalities. |
I'm hopeful there are few issues with this change, due to the way I duplicated all unittests for both safe and unsafe modes. So I'm OK with the idea of an alpha release. Once this is available, I will at least move my code to using it, and try using the The biggest problem right now is the Phobos socket tests don't work (because dub is not used, and is kind of required for using taggedalgebraic). I was hoping you might look into changing the way these tests are compiled, but maybe I can try adjusting them myself. Are there any other reviewers who might be comfortable reviewing this? I can also promote it for people who are interested on the forums. |
BTW, can you give feedback based on your partial review? i.e.:
|
Just realized, I haven't heard from you in a while @Abscissa . less than a month away from the anniversary when I started this PR ;) I think at this point, it might be good to merge and do an alpha release, and we can deal with the fallout as people use it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are my comments on what I've reviewed so far. No real objections, mostly just doc nitpicks.
I have to be honest at this point, I'm pretty well burned out on code and software dev anymore, and can barely bring myself to even touch an editor or look at a line of code these days. Because of how much I still care about and value this project, I've tried unsuccessfully to hold on and get this PR through, but the clearly unbounded delays from me are certainly not fair to this project, to its users, or to you. I've just now submitted the code review comments I had for the half of this I've already looked over. I didn't see anything major, the comments were mostly just doc-related nitpicks. It's been clear to me for quite some time that that you have a very good handle on this codebase (at least as much as I do anyway), on DLang in general, and very good developer judgement and instinct overall. Probably most of the biggest and best improvements to this project during the time I've served as its manager came either directly or indirectly from your input, and there is no doubt in my mind mysql-native wouldn't be half what it is today were it not for your suggestions and contributions. At this point, I'm only holding this project back, so I'm going to add you as an admin. You can take things from here without me holding you back. Switch it back to spaces instead of tabs if that makes things easier for you (I only changed it to tabs from spaces because it made things simpler for me.) On an ongoing basis, I'd be more than glad to offer any guidance or transition assistance that I can, on whatever you need. And I know there are still parts of this PR I've promised help on but have yet to deliver (ie, the build problem with the Phobos tests due to the new TaggedAlgebraic dependency.) I'll still try to do what I can to help out with that. |
Thanks @Abscissa I accepted your invites. It's a bit troubling to hear that you are burned out on software dev, I don't know what I'd do, since that's my actual job too! I certainly can shepherd this into release, and I'll address your concerns above. I think the biggest thing I need to do is rework the Phobos tests. |
And I'm OK with tabs as long as I can use the .editorconfig thing ;) |
ping @Abscissa can you make me an owner of the org? There are 3 other members, one who hasn't contribute AFAIK, one is Sonke (who I don't want to bother too much), and you, they are all owners. I need ownership to fix the CI problems (travis-ci isn't working correctly, it needs to be hooked to travis-ci.com). |
Also, maybe ping @s-ludwig if you have time to do it. |
"ping @Abscissa can you make me an owner of the org? " Done. (Sorry, I wasn't aware of this distinction in github.) |
What is the status of this? Just curious since it'd be nice to get rid of the deprecation warnings that come with the connection pool. Perhaps there's anything I could maybe assist with? Idk. |
I need to do some work to get it back on track. I plan to merge this soon. |
Sounds great, thanks a lot for your contribution to this! |
I still need to address comments, but reboot is #261 |
This is a pretty extensive PR. A lot of this comes from the discussion at #175, but here is a summary of what has been required for building
@safe
code that uses mysql-native.Replace Variant
std.variant.Variant
was used as a holder for any SQL parameter or result value, to ease usage of a runtime-typed language (SQL). However, due to the nature ofVariant
, and the fact that it can hold anything, it cannot be a@safe
type (the postblit and destructor are both not safe), and it also must be checked at runtime that it actually was a database-compatible type.This PR replaces usage of
Variant
withTaggedAlgebraic
. The TaggedAlgebraic type I've added in this PR (MySQLVal
) supports all types that MySQL supports, plus pointers to each of those types, and a null type. It is specifically tagged to disallow any non-@safe
calls. So while it can be used like the type it's pretending to be, it mostly should be used as a tagged union. But it does provide nice things like for instance comparing to an expected value.This version of the PR replaces ALL usages of data with MySQLVal, and provides a mechanism to revert to the original Variant type (marked deprecated). There is one issue with this path: Variant will wrap a MySQLVal. So for instance, code like
Variant v = conn.queryValue(sql);
will compile, when I wish it wouldn't. The errors you will get in your code from this will be confusing. The fix is to either change toauto
orMySQLVal
, or use theasVariant
migration function, but there is the potential to cause problems. Note that this is ONLY for reading data from the database, as mysql native will still accept Variant everywhere it did before (now marked deprecated), to allow transitioning.The "good" news here is that even though there is not a compiler error, you will most certainly get a runtime error from using the Variant-wrapped MySQLVal.
An alternative approach which I did at first, but switched to this, is to leave the Variant mechanisms as the default, deprecate them, and provide
safe
accessors to the better ones. The most awkward places for this are thequeryValue
function calls, and thegetArg
calls, which would have to have a different-named method. This leaves us in the unsavory position of having awkward naming that yes, we can deprecate later, but will be with us for a while.Variant-like harness
In mysql.types, I have added a few UFCS functions to MySQLVal to make it support a similar API to Variant (e.g.
peek
,type
,coerce
). We can likely leave these and not deprecate them, as they are possibly useful additions.Pointer support
All pointer support in MySQLVal is with const pointers. This is intentional, as the only time you use pointers is for parameter support in Prepared statements. I also do not support shared pointers, as I don't think there's any valid way to do this.
I considered a possibility that we could create a secondary algebraic type just for input parameters, to allow the result types to not have to deal with handling them, or getting in the way of operations on the algebraic. It's still a possibility we could consider.
Safe delegates
Some of the delegates used in mysql had to be marked safe. For instance the
NewConnectionDelegate
in the MySQLPool. This would be a breaking change to code that does not have safe delegates. The obvious workaround right now would be to add@trusted
to your delegates. I could support also setting@system
delegates, and just wrap them in a@trusted
call, but I felt that this decision is not one mysql-native should make, as it will completely defeat the purpose of@safe
. There is also the likelihood that the delegate can simply be marked@safe
, or already is inferred or marked@safe
, and it just works.I'm open to discussing different plans for this.
Safe unittests
I marked ALL unittests as
@safe
. In fact, I tried as much as possible to put@safe:
at the top of all modules, to try and ensure everything in mysql-native is@safe
. You will see a lot of changes in code where I get the same information but in a different way. A lot of this is still supported the old way, but is not@safe
, hence the change.Testing
I got the vibe tests to build and pass. I could NOT get the phobos code to run, as it appears to just do a straight DMD call, and the taggedalgebraic dependency is not included in that. The testing harnesses in this library are really confusing. Especially how you have compiled dmd code calling dub in weird ways! It took me a good 45 minutes to find where to update the tagged algebraic dependency.
Cleanup
I didn't squash anything or change how my commits are ordered. I also tried to update docs as much as possible. There probably is some cleanup to do, I don't want to put any effort into that until we have answers to the major decisions (safe by default? other possible issues?). So please make sure to allow me to squash away some of the changes here if you think this is OK to merge.