taocpp / taopq

C++ client library for PostgreSQL
Boost Software License 1.0
265 stars 40 forks source link

Specializing Parameter traits #57

Closed skaae closed 2 years ago

skaae commented 2 years ago

Hi, Im trying to specialize paramter_traits for an absl::CivilDay type because I want to try out the new struct to row magi :)

So far i more or less copied the string specialization an specialized it for absl::CivilDay

template<>
   struct tao::pq::parameter_traits< absl::CivilDay >
      : parameter_traits< std::string_view >
   {
       explicit parameter_traits( const absl::CivilDay v ) noexcept
         : parameter_traits<std::string_view>( absl::FormatCivilTime(v) ){}

};

FormatCivilTime will return a string. I think this works if the postgres column type is TEXT. My problem is that I'm using a DATE column type and get an error:

C++ exception with description "ERROR:  column "local_date" is of type date but expression is of type bytea
LINE 1: INSERT INTO settings VALUES ($1, $2, $3, $4, $5, $6, $7, $8,...

I tried with all the types defined in oid.hpp but none of them work. Do i need to find the oid for DATE type?

d-frey commented 2 years ago

There are several small issues with your approach. First, FormatCivilTime is not noexcept, so the ctor shouldn't be noexcept either. Next, you know it returns a std::string, but you are treating it as a std::string_view. This means the temporary string will be destroyed at the end of the ctor and the std::string_view (which is non-owning) now is dangling, meaning it points to the memory location where the string previously was and which now might contain any random garbage. Why the error message says "bytea" I can't really figure out.

Anyways, there are a couple of things to do: First, don't specialize the parameter or result traits if you don't have to. We have the simple and convenient bind-specializations that should work just fine:

template<>
struct tao::pq::bind< absl::CivilDay >
{
   [[nodiscard]] static auto to_taopq( const absl::CivilDay v )
   {
      return absl::FormatCivilTime( v );
   }

   [[nodiscard]] static auto from_taopq( const std::string_view v )
   {
      // ...
   }
};

Also, you might need to add casts to your statement's parameters, e.g. $1::DATE. Please refer to the PostgreSQL documentation as necessary.

skaae commented 2 years ago

Thanks for the pointers with string views.

Why the error message says "bytea" I can't really figure out.

I was trying to change the "type()" method to return bytea or string. But non of them worked. I accidentally copy pasted the bytea error message, but as far as i remember the the error is similar for string, but I will double check that.

Let me try your suggestion and thanks for your detailed response.

skaae commented 2 years ago

I cannot get the to/from tao to compile. I don't really understand the error mesages, but can post the code and errors if you would like to see them? Its probably me who does not understand how type traits work.

If I modify my original code to

template<>
   struct tao::pq::parameter_traits< absl::CivilDay >
      : parameter_traits< std::string >
   {
       explicit parameter_traits( const absl::CivilDay v )
         : parameter_traits<std::string>( absl::FormatCivilTime(v) ){}

};

I get the error

C++ exception with description "ERROR:  column "local_date" is of type date but expression is of type text
LINE 1: INSERT INTO settings VALUES ($1, $2, $3, $4, $5, $6, $7, $8,...
                                         ^
HINT:  You will need to rewrite or cast the expression.

I'm trying to insert into a column that is of type DATE and think that might be the issue?

d-frey commented 2 years ago

What I said before and what the error message is telling you: You need to cast the expression, something like $2::DATE instead of just $2. For the error with the to/from taopq: I forgot to switch one type, I update my previous comment. Does that fix the error for you? Also, if you only need to support CivilDay parameters (and not results), you might use the even easier:

namespace tao::pq
{
   [[nodiscard]] inline auto to_taopq( const absl::CivilDay v )
   {
      return absl::FormatCivilTime( v );
   }
}
skaae commented 2 years ago

Sorry I missed the DATE casting part. It does work if I add the cast and use your modified suggestion. The final working code is

template<>
struct tao::pq::bind< absl::CivilDay >
{
   [[nodiscard]] static auto to_taopq( const absl::CivilDay v )
   {
      return absl::FormatCivilTime( v );
   }

   [[nodiscard]] static auto from_taopq( const std::string_view v )
   {
    absl::CivilDay date;
    if (!absl::ParseCivilTime(v, &date)){
        throw std::runtime_error(fmt::format("postgres: Error parsing CivilDate: {}", v));
    };
    return date;
   }
};

This is awesome. Thank you for helping :)