search

SOCI / soci

Official repository of the SOCI - The C++ Database Access Library
http://soci.sourceforge.net/
Boost Software License 1.0
1.41k stars 477 forks source link

C++ data types support v2 #1116

Closed vadz closed 8 months ago

vadz commented 9 months ago

Unfortunately I don't have permission to push to @zann1x fork, so I had to create the new PR instead of updating #954. I've added a few small changes on top of yours and I think this should be merged now, but please let me know if you see anything wrong with what I did.

Also, I think it would probably be better to squash merge this instead of preserving the existing commits but, again, please let me know if you'd rather keep them. Thanks in advance!

zann1x commented 9 months ago

Thanks for your updates on this issue! I'm fine with almost all of the changes you've made. As you've noticed yourself, we'd yet again introduce a breakage with fdfcc12. Because of the effort we put into making the PR backwards compatible, I'd argue to not make an exception here either. I.e., I would keep the extra data_type argument in describe_column in order to be able to represent the old behavior in all cases.

vadz commented 9 months ago

I admit I hadn't realized there would be this problem when starting to make this change, and I probably wouldn't have done it if I did, but now that it's done I think we can live with the compatibility breakage just for unsigned int24 values in MySQL -- this is such a narrow corner case that it would be a pity to continue dealing with data_type in all backends for all types just to preserve compatibility with it.

If we could find some way to deal with this at MySQL level, I'd be for it, but, again, it's very annoying to have this extra parameter in all the other backends that don't need it at all. Unfortunately the only other solution I see is to add db_uint24 which is not great neither. But if we're serious about MySQL MEDIUMINT support, maybe we should do this?

zann1x commented 9 months ago

I wouldn't want to add db_uint24 just for the sake of making the MySQL backend happy.

Another solution might be to update some variable in mysql_statement_backend indicating when this special case was encountered in mysql_statement_backend::describe_column. An override of mysql_statement_backend::to_data_type could check for that variable's state and handle the case accordingly.

vadz commented 9 months ago

I wouldn't want to add db_uint24 just for the sake of making the MySQL backend happy.

I wouldn't neither, but logically speaking this would be consistent with using sized integer types for all the rest...

Another solution might be to update some variable in mysql_statement_backend indicating when this special case was encountered in mysql_statement_backend::describe_column. An override of mysql_statement_backend::to_data_type could check for that variable's state and handle the case accordingly.

This would make the function non-reentrant, which is not great neither, but it would at least be limited to MySQL backend... So let's do it like this (pushed now), thanks!