Backend inconsistencies

[cstiborg]

I recently got a change request for my application to include support for MySQL. Using SOCI I assumed that would be straight-forward. However, I ran into the issue with the line:

soci::rowset<soci::row> rowset(sql);

It constructs the object when using a PostgreSQL backend but when using MySQL it throws a soci::mysql_soci_error stating that the query is empty.

I looked into how it could be that it’s:

• Allowed in PostgreSQL but not MySQL? Simply put, PostgreSQL is happy with an empty query and simply returns an empty set for your foolishness, whereas MySQL wants to educate you on the error of your ways and throws an exception.
• Not found by a test case? All test cases for rowsets in MySQL are on the form: soci::rowset<soci::row> rowset = (sql.prepare << …); and no test for an empty rowset is performed. However, in the PostgreSQL tests this is not the case, and constructing an empty rowset is tested.

After realizing this I pondered what should be the right way to fix my problem. Whether I should handle it in my code or if I’m right in expecting that SOCI should behave identically across backends, and that throwing on a constructor, when it's provided what I consider to be valid input, is a bit weird.

My conclusion obviously (otherwise I wouldn’t be writing this) was that I would expect SOCI to behave consistently across backends, though I’m aware that meddling too much with the signalling from the backends may cause unexpected behaviour.

So I set out to produce a minimal patch. I'll attach this work to this issue and also make it available in a branch on my SOCI repo-clone. Please note that I have only patched the MySQL backend code, not the test. Along with the patch I’ve also produced a minimal piece of code to reproduce the problem and a couple of Dockerfiles, both build a test setup, one tests the original code and another tests the patched code.

The code to reproduce can be executed as follows:

# cp Dockerfile.[original|patched] Dockerfile # docker build -t original:latest . # docker run -it original

If consensus is made that this approach is sensical I will create a pull request.

soci-issue-1081.tar.gz

[vadz]

Sorry, but could you please show just the diff? Also, it's not really clear to me what happens when you construct a rowset from sql object in MySQL case, could you please explain it if you already know it?

All I can say without looking at the details is that I would indeed expect the API to behave the same with all the backends, but I would not expect SOCI from isolating you between the differences in their SQL dialects. And it's unfortunately not totally clear to me if this falls in the former or the latter case.

[cstiborg]

When you create a rowset from sqlobject using the MySQL backend (the line of code I snipped above) - the constructor will throw a soci::mysql_soci_error with the message "Query was empty". This happens because the MySQL statement code simply generates a query and passes it to the MySQL api. The API does not like empty queries and throws an exception with the message "Query was empty". I compared with the code in the PostgreSQL statement which is somewhat identical, the only difference is that the PostgreSQL api is absolutely fine with empty queries, it simply returns an empty result, quid pro quo - or rather nihil pro nihilo.

This is not an SQL dialect thing. This is SOCI telling that is doesn't like you when you create an empty rowset under MySQL. This is not the case with PostgreSQL. I haven't testet with any other backend, but it seems nonsensical to me to expose a constructor that throws when you provide proper input (in this case a valid sql session).

My patch is visible on: https://github.com/SOCI/soci/compare/master...cstiborg:soci:master

[vadz]

Sorry if I'm missing something obvious, but I don't understand how does this work at all: rowset ctor takes prepare_temp_type while sql is probably a session, so what exactly happens here? I.e. it seems to me that this code shouldn't even compile in the first place, and if it does (due to some unexpected conversions?), then this should be fixed and we shouldn't be really discussing its runtime behaviour at all...

The problem would still exist if you did

soci::rowset<soci::row> rowset(sql << "");

but this, IMO, falls into "difference between dialects" case.

[cstiborg]

This works because the rowset(prepare_temp_type) constructor is not declared explicit. So! - As the prepare_temp_type(session) constructor exists, this will implicitly be used to create the rowset object.

However, performing a soci::rowset<soci::row> rowset(sql << ""); does in fact break at compile time as sql << "" returns a once_temp_type.

As you assumed sql is indeed a session. I just modified my test to check that both: soci::rowset<soci::row> rowset(sql); and soci::rowset<soci::row> rowset(sql.prepare << ""); compiles and fails run-time using the MySQL backend, and they are also both running using the patch.

I guess my definition of what lies in the "difference between dialects" category would be anything with SQL that can be handled upstream. While soci::rowset<soci::row> rowset(sql); doesn't fit that definition as there's nothing I can do from the user side of the API, except not use it to be reassured that it won't break, soci::rowset<soci::row> rowset(sql.prepare << ""); indeed can be handled upstream and can be used in a workaround, it believe it will require some hacking, but it's not impossible.

[vadz]

So do we agree that rowset ctor should be explicit or would this break some valid use case?

I'm sorry if I'm missing something here, but to me it just doesn't make sense for rowset(sql) to compile -- what is it even supposed to do? And as I don't think we can make prepare_temp_type ctor explicit (because this would break a lot of things), I hope we can change rowset ctor to avoid it.

And if we can't do this, we probably could at least delete the ctor from session.

[cstiborg]

Yes - I'm starting to see the light now - semantically it doesn't make sense to create a rowset from a session, and I agree that it would be ideal to disallow rowset(sql). However, declaring rowset(details::prepare_temp_type const& prep) explicit would not be great as it is a conversion constructor and that would break code such as:

rowset<row> rs = (sql.prepare << "select * from soci_test")

Which is basically what all the tests test.

I guess the best bet would be to delete the rowset(session session) constructor.

[vadz]

Please check if deleting rowset(session) breaks anything, if not, let's do it. TIA!

[cstiborg]

I've just added rowset(session const & session) = delete; to rowset.h, it compiles and all tests run.

[vadz]

Please don't hesitate to make a PR with this change, TIA.