[Feedback Requested] Results returning optional, string_view, and span about sqlpp11 HOT 22 OPEN

Thanks for the feedback and the question: That would be a breaking change

from sqlpp11.

Forgot to mention: If you want to get your hands dirty and play with code, try out

https://github.com/rbock/sqlpp11/tree/optional

from sqlpp11.

OK, tried the optional branch of sqlpp and my first impressions from the changes are good!

Building sqlpp + tests (core, postgresql, mysql, sqlite) with g++ (GCC) 14.1.1 20240522 (Red Hat 14.1.1-4) went without any errors or warnings. The core, postgresql and sqlite tests also ran without any errors. The mysql tests failed because I did not have a mysql server running, so that is fine too, I guess.

Then I tried building one of my pet projects with the new sqlpp. The project builds in c++23 mode with all warnings (-Werror -Wall -Wextra) enabled. I did not notice any warnings or errors caused by c++23 compatibility issues in the sqlpp includes, which is good.

Now about the problems that I noticed.

sqlpp11-ddl2cpp seems to generate incorrect definitions for postgresql primary fields by marking them as nullable, even though they are not. For example, I have the following database definition:

CREATE TABLE tg_gap_defs (
	gap_id serial4 PRIMARY KEY,
	channel_id int8 NOT NULL REFERENCES tg_channels (channel_id),
	-- Lower end of the message gap. This message does not belong to the gap. A NULL value means that the lower end of the gap is open.
	min_message_id int8,
	-- Last fetched message ID. Fetched messages are ordered by message ID in descending order, so this field
	-- gradually decreases until it reaches min_message_id. A NULL value means that there are no fetched messages
	-- for this gap yet so the gap is open at its upper end.
	last_fetched_id int8
);
CREATE INDEX ON tg_gap_defs (channel_id);
CREATE UNIQUE INDEX ON tg_gap_defs (channel_id, min_message_id);

The generated header looks like this:

  namespace tg_gap_defs_
  {
    struct gap_id
    {
      struct _alias_t
      {
        static constexpr const char _literal[] =  "gap_id";
        using _name_t = sqlpp::make_char_sequence<sizeof(_literal), _literal>;
        template<typename T>
        struct _member_t
          {
            T gap_id;
            T& operator()() { return gap_id; }
            const T& operator()() const { return gap_id; }
          };
      };
      using _traits = sqlpp::make_traits<sqlpp::integer, sqlpp::tag::must_not_insert, sqlpp::tag::must_not_update, sqlpp::tag::can_be_null>;
    };
    struct channel_id
    {
      struct _alias_t
      {
        static constexpr const char _literal[] =  "channel_id";
        using _name_t = sqlpp::make_char_sequence<sizeof(_literal), _literal>;
        template<typename T>
        struct _member_t
          {
            T channel_id;
            T& operator()() { return channel_id; }
            const T& operator()() const { return channel_id; }
          };
      };
      using _traits = sqlpp::make_traits<sqlpp::integer, sqlpp::tag::require_insert>;
    };
    struct min_message_id
    {
      struct _alias_t
      {
        static constexpr const char _literal[] =  "min_message_id";
        using _name_t = sqlpp::make_char_sequence<sizeof(_literal), _literal>;
        template<typename T>
        struct _member_t
          {
            T min_message_id;
            T& operator()() { return min_message_id; }
            const T& operator()() const { return min_message_id; }
          };
      };
      using _traits = sqlpp::make_traits<sqlpp::integer, sqlpp::tag::can_be_null>;
    };
    struct last_fetched_id
    {
      struct _alias_t
      {
        static constexpr const char _literal[] =  "last_fetched_id";
        using _name_t = sqlpp::make_char_sequence<sizeof(_literal), _literal>;
        template<typename T>
        struct _member_t
          {
            T last_fetched_id;
            T& operator()() { return last_fetched_id; }
            const T& operator()() const { return last_fetched_id; }
          };
      };
      using _traits = sqlpp::make_traits<sqlpp::integer, sqlpp::tag::can_be_null>;
    };
  } // namespace tg_gap_defs_

So the field gap_id has property sqlpp::tag::can_be_null, which in turn means that the result field is represented by std::optional<int64_t> and trying to assign the result field to an int64_t variable in my program fails.

I guss it is more of a bug in sqlpp11-ddl2cpp and the switch to std::optional just uncovered the bug in sqlpp11-ddl2cpp which is good.

Also I noticed that the compat versions of optional, span, and string_view go directly into the sqlpp namespace instead of sqlpp::compat, unlike make_unique which goes to sqlpp::compat. Maybe it makes sense to put the new compatibility classes into sqlpp::compat in order to prevent pollution of the sqlpp namespace? Most users use c++17 or newer and they will work with the std::optional, std::string_view and std::span, anyway, so for them it won't add any extra typing on the keyboard.

from sqlpp11.

@CJCombrink If I remember correctly the SQL standard requires any PRIMARY KEY column to be NOT NULL. However I don't have the standard handy, so I cannot quote the relevant part.

From my observations pretty much any SQL database enforces this requirement, except for sqlite which as usual lives in a world of its own.

from sqlpp11.

@rbock
I like it, one question though: Will it be a breaking change or is the intention that the old syntax (dynamic_from()) will still work?

from sqlpp11.

@rbock
I think it is a great change even if it is a breaking one, because with the current codebase whenever I use sqlpp, I always end up writing boilerplate helper functions that translate between std::optional and the sqlpp value objects.

Regarding the support for std::span<uint8_t>, I am OK with it, although I must admit that I sometimes use std::string_view to hold blobs. On the other hand using std::span<uint8_t> is indeed more correct semantically when representing blobs.

std::string_view and std::span are not owning the data, so I guess the data will be released when the corresponding row object is destroyed?

Also I assume nullable text columns will be represented by std::optional<std::string_view> ?

from sqlpp11.

Regarding the dynamic queries, just like everyone else I try to avoid them and only use them when static queries would cause too much code duplication :-)

Overall depending on the project, dynamic queries are somewhere in the range of 2-5% of all sqlpp queries in my code. So a breaking change in the dynamic queries in not a major issue for me.

However I just want to confirm - does this change mean that the old way of adding fields to query clauses will not longer work? That is there would be no way to add select fields in a separate if branch like in your example:

if (use_omega)
{
   s.selected_columns.add(without_table_check(f.omega));
   s.from.add(dynamic_cross_join(f));
}

This is not a big issue, but I want to clarify that issue.

from sqlpp11.

Thanks for the detailed feedback!

std::string_view and std::span are not owning the data, so I guess the data will be released when the corresponding row object is destroyed

It might be destroyed with the row or with the result, it depends on the database backend. The safe strategy would be to assume destruction with the row.

However I just want to confirm - does this change mean that the old way of adding fields to query clauses will not longer work? That is there would be no way to add select fields in a separate if branch like in your example:

That is correct.

from sqlpp11.

std::string_view and std::span are not owning the data, so I guess the data will be released when the corresponding row object is destroyed

It might be destroyed with the row or with the result, it depends on the database backend. The safe strategy would be to assume destruction with the row.

OK, this is what I assumed too. It probably makes sense to document this behavior because I am pretty sure that this question will be asked again and again.

However I just want to confirm - does this change mean that the old way of adding fields to query clauses will not longer work? That is there would be no way to add select fields in a separate if branch like in your example:

That is correct.

OK, I think that at least in my case it is relatively easy to adjust my code to the new syntax.

I am really happy with the new std/sqlpp containers because they will make the users' code much simpler and more straightforward. This weekend I will try modifying and building one of my bigger projects with the new sqlpp and will report any success, issues and/or suggestions.

from sqlpp11.

Everything sounds good. I think our code base uses dynamic queries more than they should which will make adoption a bit harder.

But I would not let breaking changes stand in the way of better/cleaner code. And if it means a clearer path to something like sqlpp17/20/23 then it is a step worth taking.

Maybe consider releasing version 1.0 before the change and then this would become version 2.0.
Not that I am suggesting you maintain two version, just to make it clear that the API is breaking form X to Y.

from sqlpp11.

Thanks for mentioning the release version. I'll certainly think about it.

I wonder how the transition would work? There is a release called 1.0, but how would the somewhat experimental code be called before marking 2.0? Keep it in the current branch? Call it 2.alpha.?

from sqlpp11.

I have no experience with releasing public libraries but I can imagine keeping the work in a version_2 branch until mature and then merging into main and release version 2.0.
Then at least you can support the current version 1 (in themain branch) if there are major bugs or issues
then when you are happy with 2 it gets merged into main, 2 is released and you stop supporting version 1.
You can then make alpha and beta releases from the version_2 branch for testing

from sqlpp11.

I wonder how the transition would work? There is a release called 1.0, but how would the somewhat experimental code be called before marking 2.0? Keep it in the current branch? Call it 2.alpha.?

One option would be:

• Have a branch called stable-0.x. This is the current main branch. This branch gets only bugfixes.
• Have a branch called dev-1.x. This is the current optional branch. This branch gets bugfixes and new features.

Once the dev-1.x is stabilized, rename dev-1.x to stable-1.x, then fork stable-1.x as dev-2.x and continue adding new features to dev-2.x. Once dev-2.x gets stabilized, rename it to stable-2.x and fork it as dev-3.x, then rinse and repeat forever.

Or maybe don't version the development branch and just have one dev branch, which will be essentially our current main branch. That is the branch structure will look like this:

stable-0.x
stable-1.x
stable-2.x
dev

In the above example the dev branch will have versions 3.x.

from sqlpp11.

Thanks for the suggestions!

@MeanSquaredError That's very close to the gitflow model, IIUC. My life's much easier since I dropped that in favor of just one main branch :-)

It is not super urgent. It will take a few weeks at least before all of the above is done. For the time being, there will be main and optional.

from sqlpp11.

@MeanSquaredError Your investigations are really useful and inspiring. I will also try to use the new branch in the coming week if possible.

So I wrote a long response on how PRIMRAY KEY does not imply primary key, then went on to test and indeed you are correct, also according to the docs:

The PRIMARY KEY constraint specifies that a column or columns of a table can contain only unique (non-duplicate), nonnull values

PostgreSQL: CREATE TABLE

from sqlpp11.

Thanks for playing with the optional branch and the detailed feedback, @MeanSquaredError !

• ddl2cpp is unchanged, so this surfaced an existing bug, it seems.
• Good observation regarding the compatibility classes. Moving makes sense. Added a TODO locally.

My local branch is a bit further along, I knocked away all dynamic code and started to remove quite some additional fluff in the clauses which became superfluous now. I'll upload as soon as tests compile again.

The next step would then be to add optional columns, joins, expressions (probably in that order).

from sqlpp11.

@rbock
Tomorrow I will prepare a PR with a fix for this bug in ddl2cpp.

On a side note, it seems that the optional branch at github lacks this commit from the main branch:
b92a9a7

from sqlpp11.

On a side note, it seems that the optional branch at github lacks this commit from the main branch: b92a9a7

Thanks, will come with the next upload.

from sqlpp11.

Added a new branch: optional-no-dynamic.

• Rebased on main.
• Squashed optional branch
• Removed dynamic query parts
• Massive cleanup of superfluous classes and functions (wanted to get this out of the way before adding new stuff).
• Moved compatibility classes into compat namespace

Next I'll look into adding optional columns (or additional cleanup potential).

from sqlpp11.

@rbock
Thanks for the new version. It built without a problem, apart from a bunch of warnings in the tests. I assume that the warnings are expected as most of them are due to #warning directives.

Then I tried building one of my personal projects with the new sqlpp. It seems that it has a problem assigning an std::optional<int64> value to a nullable field. This is an extract of the errors that it shows during the build:

In file included from /usr/local/include/sqlpp11/table.h:32,
                 from /usr/local/include/sqlpp11/select_pseudo_table.h:30,
                 from /usr/local/include/sqlpp11/postgresql/returning_column_list.h:39,
                 from /usr/local/include/sqlpp11/postgresql/returning.h:30,
                 from /usr/local/include/sqlpp11/postgresql/insert.h:31,
                 from /usr/local/include/sqlpp11/postgresql/postgresql.h:34,
                 from /usr/local/projects/private/unreal/source/database/db_manager.h:4,
                 from /usr/local/projects/private/unreal/source/database/db_global.h:3,
                 from /usr/local/projects/private/unreal/source/database/db_use.h:3,
                 from /usr/local/projects/private/unreal/source/telegram/tg_gap_filler.h:3,
                 from /usr/local/projects/private/unreal/source/telegram/tg_gap_filler.cpp:1:
/usr/local/include/sqlpp11/column.h: In instantiation of ‘sqlpp::assignment_t<sqlpp::column_t<Table, ColumnSpec>, typename sqlpp::wrap_operand<T, void>::type> sqlpp::column_t<Table, ColumnSpec>::operator=(T) const [with T = std::optional<long int>; Table = db_model::tg_gap_defs; ColumnSpec = db_model::tg_gap_defs_::min_message_id; typename sqlpp::wrap_operand<T, void>::type = std::optional<long int>]’:
/usr/local/projects/private/unreal/source/telegram/tg_gap_filler.cpp:48:57:   required from here
   48 |                         tggd.min_message_id = get_last_message_id (channel_id),
      |                                                                              ^
/usr/local/include/sqlpp11/column.h:93:56: error: static assertion failed: invalid rhs assignment operand
   93 |       static_assert(_is_valid_assignment_operand<rhs>::value, "invalid rhs assignment operand");
      |                                                        ^~~~~
/usr/local/include/sqlpp11/column.h:93:56: note: ‘sqlpp::is_valid_assignment_operand<sqlpp::integral, std::optional<long int>, void>::value’ evaluates to false

The function get_last_message_id returns a std::optional<int64_t>

This is the relevant part of the database model generated by dll2cpp:

  namespace tg_gap_defs_
  {
    struct gap_id
    {
      struct _alias_t
      {
        static constexpr const char _literal[] =  "gap_id";
        using _name_t = sqlpp::make_char_sequence<sizeof(_literal), _literal>;
        template<typename T>
        struct _member_t
          {
            T gap_id;
            T& operator()() { return gap_id; }
            const T& operator()() const { return gap_id; }
          };
      };
      using _traits = sqlpp::make_traits<sqlpp::integer, sqlpp::tag::must_not_insert, sqlpp::tag::must_not_update>;
    };
    struct channel_id
    {
      struct _alias_t
      {
        static constexpr const char _literal[] =  "channel_id";
        using _name_t = sqlpp::make_char_sequence<sizeof(_literal), _literal>;
        template<typename T>
        struct _member_t
          {
            T channel_id;
            T& operator()() { return channel_id; }
            const T& operator()() const { return channel_id; }
          };
      };
      using _traits = sqlpp::make_traits<sqlpp::integer, sqlpp::tag::require_insert>;
    };
    struct min_message_id
    {
      struct _alias_t
      {
        static constexpr const char _literal[] =  "min_message_id";
        using _name_t = sqlpp::make_char_sequence<sizeof(_literal), _literal>;
        template<typename T>
        struct _member_t
          {
            T min_message_id;
            T& operator()() { return min_message_id; }
            const T& operator()() const { return min_message_id; }
          };
      };
      using _traits = sqlpp::make_traits<sqlpp::integer, sqlpp::tag::can_be_null>;
    };
    struct last_fetched_id
    {
      struct _alias_t
      {
        static constexpr const char _literal[] =  "last_fetched_id";
        using _name_t = sqlpp::make_char_sequence<sizeof(_literal), _literal>;
        template<typename T>
        struct _member_t
          {
            T last_fetched_id;
            T& operator()() { return last_fetched_id; }
            const T& operator()() const { return last_fetched_id; }
          };
      };
      using _traits = sqlpp::make_traits<sqlpp::integer, sqlpp::tag::can_be_null>;
    };
  } // namespace tg_gap_defs_

I am not sure if this information is sufficient, so if you want, I can prepare a minimal example tomorrow.

from sqlpp11.

Thanks for testing. And sorry, I apparently should have said this clearer: This new branch is strictly less than the optional branch. I ripped out all dynamic parts and a lot of helper classes and functions that turned out to be superfluous.

Adding support for using optional in queries is yet to come (but it will be much easier after this cleanup).

from sqlpp11.

Thanks for testing. And sorry, I apparently should have said this clearer: This new branch is strictly less than the optional branch. I ripped out all dynamic parts and a lot of helper classes and functions that turned out to be superfluous.

Adding support for using optional in queries is yet to come (but it will be much easier after this cleanup).

OK, thanks for the clarification! No hurry - the current stable version works great and I am pretty sure that the new version will be even better. If you need more testing for your work-in-progress, just post an update here and I will test it again.

from sqlpp11.