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

Throw if truncation happens but no indicators was provided #51

Closed
vadz opened this issue Feb 3, 2013 · 3 comments
Closed

Throw if truncation happens but no indicators was provided #51

vadz opened this issue Feb 3, 2013 · 3 comments
Assignees
Milestone

Comments

@vadz
Copy link
Member

vadz commented Feb 3, 2013

Currently if a select statement results in a null value and no indicator is defined for the corresponding output parameter, a "Null value fetched and no indicator defined." exception is thrown. However when the same happens with truncation, it is silently ignored if there is no indicator (if there is one, the truncation is reported via it). This is inconsistent and actually dangerous as the data can be silently corrupted, so we should throw an exception when this happens instead of just ignoring it.

As this would be a backwards incompatible change, it should only be done in 4.0 and not in any of 3.x releases.

@ghost ghost assigned vadz Feb 3, 2013
@vadz
Copy link
Member Author

vadz commented Jun 6, 2014

So I've finally started working on this. The bad news is that actually no backend even sets the indicator correctly currently, here is the result of the tests:

  1. Firebird: indicator is i_ok (the value is, of course, still truncated).
  2. MySQL: indicator is i_ok.
  3. ODBC using PostgreSQL backend: exception thrown.
  4. PostgreSQL: exception thrown.
  5. SQLite: indicator is i_ok.

I can't test Oracle which is a pity because, looking at the code, this is the only backend which has some code for returning i_truncated (ODBC backend also has it but clearly this doesn't work there).

Anyhow, I still think that the current situation is wrong, but it's even wronger than I thought and my changes will break the code for all backends (possibly excluding Oracle) as not only will the indicator be set now if available, but also the exception currently thrown by PostgreSQL backend will not be thrown any more. If anybody objects against changing this and sees some other way of fixing this in a more backward compatible manner, please comment.

@vadz
Copy link
Member Author

vadz commented Apr 20, 2015

Update: I have now tested with Oracle too and it behaves as PostgreSQL, i.e. an exception is thrown.

So I actually have a new plan: just make all backends throw an exception on truncation except, perhaps, SQLite, which doesn't lose data (even though it doesn't truncate neither).

The main problem I have with this is that I don't know how to make MySQL generate an exception in non-strict mode, would anybody know how to do it? If not, I'll probably just leave MySQL alone.

vadz added a commit to vadz/soci that referenced this issue Apr 21, 2015
Also add a unit test verifying that an exception is indeed thrown if the value
being inserted doesn't fit in the column, except for SQLite where anything
fits and MySQL where it doesn't but just gets silently truncated by the
database by default.

Closes SOCI#51.
@mloskot
Copy link
Contributor

mloskot commented Apr 21, 2015

The new plan sounds good 👍

I don't know. @pfedor is our MySQL guru.

vadz added a commit to vadz/soci that referenced this issue Apr 21, 2015
Silently truncating the data to fit into a column is a bad idea, just don't do
this.

Also add a unit test verifying that an exception is indeed thrown if the value
being inserted doesn't fit in the column, except for SQLite where anything
fits and MySQL where it doesn't but just gets silently truncated by the
database by default.

Finally add a helper on_after_ddl() method to the test context class to allow
creating tables in the common test code instead of having to always do it in
RDBMS-specific way, even when the DDL uses completely standard SQL and the
only difference is that Firebird needs a commit after executing it.

Closes SOCI#51.
@vadz vadz closed this as completed in bc4abd7 Apr 22, 2015
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

Successfully merging a pull request may close this issue.

2 participants