Backend inconsistencies

[cstiborg]

Backend inconsistencies

[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: 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.