jtv / libpqxx

The official C++ client API for PostgreSQL.
http://pqxx.org/libpqxx/
BSD 3-Clause "New" or "Revised" License
1.04k stars 241 forks source link

missing nonnull condition in exec_prepared() function? #373

Closed AttilaHorvath-PP closed 4 years ago

AttilaHorvath-PP commented 4 years ago

Version: 6.4 I have a simple project working with prepared statements. After I switched to this version I found out the prepared() function is deprecated now and I should use exec_prepared() function to execute prepared statements. I couldn't find any documentation but my guess is the "nonnull" condition got removed in the new function. I'm working with a lot of pointer types and I want to use them in prepared insert like: "value behind the pointer if the pointer is not null and null otherwise". Is there any way to do so? As and nullptr type will most likely differ I can't use the "?:" operator. Thanks in advance!

tt4g commented 4 years ago

You can use pqxx::connection_base::prepare() and pqxx:transaction_base::exec_prepared() in pqxx 6.4.x. pqxx::connection_base::prepare() is save SQL statement with name, pqxx:transaction_base::exec_prepared() will call SQL statement saved by pqxx::connection_base::prepare().

See also API document:

Another way, use pqxx:transaction_base::exec_params(). pqxx:transaction_base::exec_params() executes the statement immediately, without naming it.

I couldn't find any documentation but my guess is the "nonnull" condition got removed in the new function. I'm working with a lot of pointer types and I want to use them in prepared insert like

As far as I remember, when the pointer is NULL it automatically assigns NULL to the statement parameter. However, some types may not have been assigned `NULL' in pqxx 6.x.

https://github.com/jtv/libpqxx/blob/ff7bf80ea942fd91a6ce526405ecbc64131f634f/test/unit/test_prepared_statement.cxx#L278-L287

AttilaHorvath-PP commented 4 years ago

Sorry my first post could've been a bit misleading so I'm trying to go into details.

pqxx::transaction_base::prepared: here pqxx::prepare::invocation:here

The most important member function for me is this one (in invocation class):

template<typename T >
invocation & operator() (const T &v, bool nonnull)

Combining these two you could do something like this:

txn.prepared("whatever")(y->z->getA(), y->z != nullptr).exec()

In this case the prepared statement would be executed like this:

But now this whole method is deprecated. My question: is there any similar way to work with pointer types in a convenient way? Attached a test file with examples

tt4g commented 4 years ago

As far as I know, the feature you are looking for (conditional binding) is not provided.

Workaround: You should be able to achieve the function you are looking for with a home-made wrapper function.

Example: ```cpp #include #include #include class B { public: B():c(2){} int c; }; class A { public: A(): b(nullptr){} std::shared_ptr b; }; class SafeBinding { public: explicit SafeBinding(const A& a) : a_(a) { } int* get() const { if (this->a_.b == nullptr) { return nullptr; } else { return &(this->a_.b->c); } } private: const A& a_; }; int main() { A myVar1; A myVar2; myVar2.b = std::make_shared(); pqxx::connection c("user='x' password='y' host='localhost'"); c.prepare("myprepared", "SELECT $1"); pqxx::work w(c); SafeBinding safeBindingMyVar1(myVar1); auto result = w.prepared("myprepared")(safeBindingMyVar1.get()).exec(); //expected to call the statement with NULL std::cout << "#1: " << (result[0][0].is_null() ? "null" : result[0][0].as()) << std::endl; SafeBinding safeBindingMyVar2(myVar2); auto result2 = w.prepared("myprepared")(safeBindingMyVar2.get()).exec(); //expected to call the statement with 2 std::cout << "#2: " << (result2[0][0].is_null() ? "null" : result2[0][0].as()) << std::endl; return 0; } ```

WandBox

jtv commented 4 years ago

Yes, that "is it null" condition is deprecated. Calling prepared or parameterised statements is very different now: one C++ parameter simply matches to one SQL parameter, in a function with a variable arguments list.

By the way, consider upgrading to 7.x. The latest released version is 7.2.0.

Then... to pass a value that may be null, you could pass it as a std::optional, or a std::shared_ptr, or std::unique_ptr.

jtv commented 4 years ago

Have a bit more time to read the conversation now.

ISTM the old "not null" parameter could not make y->z->getA() valid when y->z is null in any case. Not without playing funny games with overloaded operator->() I guess. Otherwise there's just no way around the null pointer crash when you dereference y->z.

But as @tt4g suggested, you could perhaps write a useful wrapper for...

    (y->z == nullptr) ? std::optional<Foo>() : std::optional<Foo>(y->z->getA())

And the nice thing about that is it doesn't need anything special from libpqxx. If you have the value above in a variable myA, you could then just do:

    auto rows = tx.exec_prepared(statement, myA);
AttilaHorvath-PP commented 4 years ago

@jtv I wanted to get y->z->getA() when y->z is not null. But anyways I get the problem and it's quite unfortunate for me. I can't really upgrade to 7.x because I'm bound to gcc version 5.4 (cross-compilation) and C++17 features are not present there (only a very limited set). Thank you very much for your help guys. Seems like I have to work around it somehow.

jtv commented 4 years ago

That's a shame. I think the wrapper idea would probably still work though.

AttilaHorvath-PP commented 4 years ago

As much as I know the std::optional is a C++ 17 feature. I ran a search and found that std::experimental::optional is available in older gcc versions. I might give it a try.

Is there any way to make exec_prepared() write NULL to a smallint field other than passing nullptr as argument? Like "" works for varchar fields.

jtv commented 4 years ago

You can write your own conversions for your own C++ types, by specialising the string_traits template for them. (And a bit more in libpqxx 7.x.) You could write one that takes an int *, for instance.

AttilaHorvath-PP commented 4 years ago

I'm trying to specialize string_traits for one of my object first but I got some compilation error. If you could give me a hint I would appreciate it :)

Code at the beginning of my own header:

namespace pqxx {
  template <>
  struct string_traits<myclass *> {
    static constexpr const char *name( ) noexcept {
      return "myclass *";
    }
    static constexpr bool has_null( ) noexcept {
      return true;
    }
    static bool is_null( myclass *st ) {
      return st == nullptr;
    }
    static myclass *null( ) {
      return nullptr;
    }
    static void from_string( const char Str[], myclass& *st ) {
      auto stint = std::stoi( Str );
      st = new myclass( stint );
      else {
        throw std::runtime_error( "not recognized type" );
      }
    }
    static std::string to_string( const myclass *&st ) {
      return std::to_string( myclass->toBitMask( ) );
    }
  };

  namespace internal {
    template <>
    struct type_name<myclass *> {
      static constexpr const char *value = "myclass *";
    };
  } // namespace internal
} // namespace pqxx

I'm having trouble compiling this probably I'm still missing something. Got the following:

 #1
/usr/local/include/pqxx/field.hxx: In instantiation of ‘T pqxx::field::as() const [with T = myclass*]’:
/home/attila/dev/repos/myproject/src/myclass.cpp:83:103:   required from here
/usr/local/include/pqxx/field.hxx:186:9: error: no matching function for call to ‘pqxx::field::to(myclass*&) const’
     if (not to(Obj)) Obj = string_traits<T>::null();
/usr/local/include/pqxx/field.hxx:135:29: note: candidate: template<class T> typename std::enable_if<((! std::is_pointer<_Tp>::value) || std::is_same<T, const char*>::value), bool>::type pqxx::field::to(T&) const
   template<typename T> auto to(T &Obj) const    //[t03]
/usr/local/include/pqxx/field.hxx:155:29: note:   template argument deduction/substitution failed:
/usr/local/include/pqxx/field.hxx:186:9: note:   candidate expects 2 arguments, 1 provided
     if (not to(Obj)) Obj = string_traits<T>::null();
 #2
In file included from /usr/local/include/pqxx/internal/statement_parameters.hxx:28:0,
                 from /usr/local/include/pqxx/prepared_statement.hxx:18,
                 from /usr/local/include/pqxx/connection_base.hxx:26,
                 from /usr/local/include/pqxx/basic_connection.hxx:23,
                 from /usr/local/include/pqxx/connection.hxx:20,
                 from /usr/local/include/pqxx/connection:6,
                 from /usr/local/include/pqxx/pqxx:4,
                 from /home/attila/dev/repos/sagateway/source/libEndPointWebSCADA/src/utils/DataPointLogDao.h:8,
                 from /home/attila/dev/repos/myproject/src/myclass.cpp:5:
/usr/local/include/pqxx/internal/type_utils.hxx: In instantiation of ‘static bool pqxx::string_traits<T, typename std::enable_if<pqxx::internal::is_optional<T>::value>::type>::is_null(const T&) [with T = myclass*]’:
/usr/local/include/pqxx/internal/statement_parameters.hxx:192:36:   required from ‘void pqxx::internal::params::add_field(const Arg&) [with Arg = myclass*]’
/usr/local/include/pqxx/internal/statement_parameters.hxx:215:5:   recursively required from ‘void pqxx::internal::params::add_fields(Arg&&, More&& ...) [with Arg = long unsigned int&; More = {long unsigned int&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, myclass*, myclass*, unsigned int}]’
/usr/local/include/pqxx/internal/statement_parameters.hxx:215:5:   required from ‘void pqxx::internal::params::add_fields(Arg&&, More&& ...) [with Arg = unsigned int&; More = {long unsigned int&, long unsigned int&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, myclass*, myclass*, unsigned int}]’
/usr/local/include/pqxx/internal/statement_parameters.hxx:118:5:   required from ‘pqxx::internal::params::params(Args&& ...) [with Args = {unsigned int&, long unsigned int&, long unsigned int&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, myclass*, myclass*, unsigned int}]’
/usr/local/include/pqxx/transaction_base.hxx:399:28:   required from ‘pqxx::result pqxx::transaction_base::exec_prepared(const string&, Args&& ...) [with Args = {unsigned int&, long unsigned int&, long unsigned int&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, myclass*, myclass*, unsigned int}; std::__cxx11::string = std::__cxx11::basic_string<char>]’
/home/attila/dev/repos/myproject/src/myclass.cpp:40:115:   required from here
/usr/local/include/pqxx/internal/type_utils.hxx:187:49: error: incomplete type ‘pqxx::string_traits<myclass, void>’ used in nested name specifier
     { return (not v || string_traits<I>::is_null(*v)); }
                        ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~

... and I'm getting similar with

/usr/local/include/pqxx/internal/type_utils.hxx:184:36: error: incomplete type ‘pqxx::string_traits<SAGateway::WebSCADA::WebSCADADataPointStatus, void>’ used in nested name specifier
     { return string_traits<I>::name(); }
              ~~~~~~~~~~~~~~~~~~~~~~^~
.
.
.
/usr/local/include/pqxx/internal/type_utils.hxx:205:39: error: incomplete type ‘pqxx::string_traits<SAGateway::WebSCADA::WebSCADADataPointStatus, void>’ used in nested name specifier
     return string_traits<I>::to_string(*Obj);

I guess I didn't get something or there are more things I have to specialize so if you can help me that would be great! :)

(ps.: my original goal was to make it work with std::shared_ptr objects but somehow the shared_ptr got removed inside the library so I changed to raw poitners)

jtv commented 4 years ago

(Typo: from_string takes a myclass & * — should be myclass* &.)

The errors puzzle me, but then again it's been a while since I dived into the pre-7.x string conversion machinery.

Could it be that your string_traits specialisation was simply not visible to the code at the point where you used it?

AttilaHorvath-PP commented 4 years ago

Copied from field.hxx (line 135)

 template<typename T> auto to(T &Obj) const             //[t03]
    -> typename std::enable_if<(
      not std::is_pointer<T>::value
      or std::is_same<T, const char*>::value
    ), bool>::type
  {
    const char *const bytes = c_str();
    if (bytes[0] == '\0' and is_null()) return false;
    from_string(bytes, Obj);
    return true;
  }

I'm not that experienced with template magic but I'm afraid this won't work with pointer types except const char *.

tt4g commented 4 years ago

I think it's pqxx::field::c_str().

https://github.com/jtv/libpqxx/blob/ff7bf80ea942fd91a6ce526405ecbc64131f634f/include/pqxx/field.hxx#L115-L120

I remember that pqxx::field::c_str() calls pqxx::string_traits::to_string().

In pqxx 6.x, most pqxx::string_traits are declared in https://github.com/jtv/libpqxx/blob/6.4.7/include/pqxx/strconv.hxx See the specialization of pqxx::string_traits.

AttilaHorvath-PP commented 4 years ago

I can't compile the code because the following part won't let me - as far as I understand.

typename std::enable_if<(
      not std::is_pointer<T>::value
      or std::is_same<T, const char*>::value
    ), bool>::type

My type as you can see in the previous comments (myclass *) *is a pointer type and it's not` const char ** so the boolean instd::enable_ifwill evaluate to false. That's why I come to a conclusion I can't possibly use it with pointer types (except the above mentionedconst char *` ) You can see the compilation error message in a comment above from 2 days ago.

tt4g commented 4 years ago

You must declare specialised string_traits in the class you are declaring in your application.

Roughly, the implementation should be as follows (sorry, I have not verified that this code works.):

class B {
public:
    B():c(2){}
    int c;
};

class A {
public:
    A(): b(nullptr){}
    std::shared_ptr<B> b;
};

namespace pqxx
{

template<> struct PQXX_LIBEXPORT string_traits<A>
{
  static constexpr const char *name() noexcept
    { return "A"; }
  static constexpr bool has_null() noexcept { return true; }
  static bool is_null(A a) { return bool(a.b); }
  static A* null() { return nullptr; }
  static void from_string(const char Str[], A &a)
  {
    int c;
    pqxx::string_traits::from_string<int>(Str, c);

    a.b = std::make_shared<B>(c);
  }
  static std::string to_string(A Obj)
  {
    return pqxx::string_traits::to_string(a.b->c);
  }
}

} // namespace pqxx

Implementation by the wrapper may be easier to understand than specialization.

jtv commented 4 years ago

I think @AttilaHorvath-PP is right. That "is it a pointer?" logic strikes me as overly broad, and at this moment I can't think why we did it that way.

Maybe we should even just generally say "if it's a pointer to T, then we should treat it as either a null or a T." And of course we'd have to do that in both 6.x and 7.x.

But I'll have to do some digging to reconstruct the reasoning behind that enable_if. Presumably something would go wrong if we didn't have it!

jtv commented 4 years ago

Just took some time to think this over, and... I now see why "pointer types other than char const *" are disallowed here. It's all fine returning a pointer, but who holds the actual object to which it points?

If your from_string() allocates the object, then it should provide for its deallocation as well. And the appropriate way to do that is to return a smart pointer: shared_ptr or unique_ptr — not a raw pointer.

I'll need to document this, because it's not clear.

tt4g commented 4 years ago

IMO.

I expect that pointer-to-pointer will have a similar problem.

The C++ compiler does identify local arrays, but it's difficult to distinguish whether a pointer-to-pointer given as an argument is a pure set of pointers or an array of pointers.

In most cases, it is the programmer's responsibility to work around this problem. In order for an API to be able to accept both, you need to have an array-only API and a pointer-to-pointer-only API, respectively.

Nevertheless, it would take a lot of work for a library like pqxx that goes through multiple templates to support all use cases.

AttilaHorvath-PP commented 4 years ago

@jtv Thank you very much for spending your time on my issue.

What I really wanted at the beginning was working with std::shared_ptr<myclass> but I had another issue with that. Copied from _include/pqxx/internal/type_utils.hxx (starting at line 175)_:

// TODO: Move?
namespace pqxx
{

/// Meta `pqxx::string_traits` for std::optional-like types.
template<typename T> struct string_traits<
  T,
  typename std::enable_if<internal::is_optional<T>::value>::type
>
{
private:
  using I = internal::inner_type<T>;
public:
  static constexpr const char *name() noexcept
    { return string_traits<I>::name(); }
  static constexpr bool has_null() noexcept { return true; }
  static bool is_null(const T& v)
    { return (not v || string_traits<I>::is_null(*v)); }
  static constexpr T null() { return internal::null_value<T>(); }
  static void from_string(const char Str[], T &Obj)
  {
    if (not Str) Obj = null();
    else
    {
      I inner;
      string_traits<I>::from_string(Str, inner);
      // Utilize existing memory if possible (e.g. for pointer types).
      if (Obj) *Obj = inner;
      // Important to assign to set valid flag for smart optional types.
      else Obj = internal::make_optional<T>(inner);
    }
  }
  static std::string to_string(const T& Obj)
  {
    if (is_null(Obj)) internal::throw_null_conversion(name());
    return string_traits<I>::to_string(*Obj);
  }
};

} // namespace pqxx

I suspect using I = internal::inner_type<T>; statement gets rid of std::shared_ptr and transforms std::shared_ptr<T> to T in this line { return (not v || string_traits<I>::is_null(*v)); }. Same file (line 30)

/// Extract the content type held by an `optional`-like wrapper type.
/* Replace nested `std::remove_*`s with `std::remove_cvref` in C++20 */
template<typename T> using inner_type = typename std::remove_cv<
  typename std::remove_reference<
    decltype(*std::declval<T>())
  >::type
>::type;

Of course I don't have string_traits specialized for myclass (because I want it to work with std::shared_ptr<myclass>) so I get an "incomplete type" error.

I really wonder if I'm the first one who wants to work with pointer types (either smart or raw). I might try to modify the library source code and see if I can get somewhere. Still... if you have any hints it's appreciated :)

jtv commented 4 years ago

Well if I were you I'd start off writing a string_traits<myclass>. It won't have a null value, but that's fine, because shared_ptr<myclass> will. I'd do that and write a few tests with to_string() and from_string().

We currently have a generic template specialisation for string_traits<std::shared_ptr<T>>... Unfortunately that's not in the libpqxx version you are using, but I don't think it'd be hard to reproduce in libpqxx 6.4. Put together, those two would give you what you need.

The bad news is that once you want to upgrade to libpqxx 7, there will be changes in the string conversion API. On the flip side, it's also better documented.

AttilaHorvath-PP commented 4 years ago

While I was adapting string_traits<std::shared_ptr<T>> to 6.4.7 I found a piece of code in the library tests (with the help of compiler errors). Copied from test/unit/test_prepared_statement.cxx (at the end of the file):

/// Test against any optional type, such as std::optional<int> or 
/// std::experimental::optional<int>.
template<typename Opt>
void test_optional(transaction_base &T)
{
  T.conn().prepare("EchoNum", "SELECT $1::int");
  pqxx::row rw = T.exec_prepared1(
    "EchoNum",
    pqxx::internal::make_optional<Opt>(10)
  );
  PQXX_CHECK_EQUAL(
    rw.front().as<int>(),
    10,
    "optional (with value) did not return the right value.");

  rw = T.exec_prepared1("EchoNum", Opt());
  PQXX_CHECK(
    rw.front().is_null(),
    "optional without value did not come out as null.");
}

void test_prepared_statements()
{
  connection conn;
  work tx{conn};
  test_registration_and_invocation(tx);
  test_basic_args(tx);
  test_multiple_params(tx);
  test_nulls(tx);
  test_strings(tx);
  test_binary(tx);
  test_dynamic_params(tx);

#if defined(PQXX_HAVE_OPTIONAL)
  test_optional<std::optional<int>>(tx);
#elif defined(PQXX_HAVE_EXP_OPTIONAL) && !defined(PQXX_HIDE_EXP_OPTIONAL)
  test_optional<std::experimental::optional<int>>(tx);
#endif
  test_optional<std::unique_ptr<int>>(tx);
  test_optional<std::shared_ptr<int>>(tx);
}

Got my hopes high so I implemented string_traits<myclass> and tried something like this: tx.exec_prepared("my_insert", a, b, pqxx::internal::make_optional<std::shared_ptr<myclass>>( p ));

I'm getting: error: call of overloaded ‘make_optional<....> ' is ambigous candidates: /usr/local/include/pqxx/internal/type_utils.hxx:143:49: /usr/local/include/pqxx/internal/type_utils.hxx:159:49:

So it could work (~compile) with two different specialization of the template. One is for optional-like types and the other one is for shared_ptr. I'm about to compile and run the library tests on my local machine but haven't figured out how to supply database credentials to the test program yet. I'm getting the following exception:

Running: test_prepared_statement
Exception: fe_sendauth: no password supplied
tt4g commented 4 years ago

I'm about to compile and run the library tests on my local machine but haven't figured out how to supply database credentials to the test program yet. I'm getting the following exception:

Try set PGPASSWORD environment variable.

jtv commented 4 years ago

Or use "peer authentication" if you're connecting to your local machine on a Unix domain socket. That simply checks your user identity in the database against your user identity on the system itself. Safer and more convenient than passwords.

AttilaHorvath-PP commented 4 years ago

Thanks I could make it work setting PGUSER and PGPASSWORD environment variables.

My issue has been resolved as well. It was also my fault/mistake (with the template specialization and includes). Conclusion: if you want to use std::shared_ptr<myclass> as exec_prepared() parameter you can make it work and you just need a specialization of myclass. It's not obvious but the library can handle the std::shared_ptr part. It gave me a trouble to find it out but as I said I also made mistakes myself. I still haven't tested it that much though. Guess it will be fine. Thanks @jtv and @tt4g for your help and patience.

jtv commented 4 years ago

Glad to hear the problem is solved. I wasn't sure about the std::shared_ptr support in that version. Of course I'm usually focused on the latest and greatest version.

I'll have to change that in the future though, because C++20 is just too good to wait. So... more maintenance of parallel versions until everyone has the newer compilers.

Time to close this bug. Feel free to file a new one if there's any more trouble. But also, look for compiler upgrades! :-)

juzzlin commented 3 years ago

What would be the exec_prepared() equivalent for deprecated form prepared(myStatement)(myIntVar1)(myIntVar2ThatCanBeNull, isNull).exec() ?

jtv commented 3 years ago

Instead of passing an int, you can pass a std::optional<int>, or a std::unique_ptr<int>, or a std::shared_ptr<int>. If it contains no value (e.g. a default-constructed std::optional<int>), then that's an SQL null.

Plus, in the case of a char *, of course a null pointer also translates to an SQL null.

jtv commented 3 years ago

Oh, and there is now also pqxx::params. There, the same thing works but you also get another alternative:

    pqxx::params p;
    p.append(myIntVar1);
    if (isNull) p.append();
    else p.append(myIntVar2ThatCanBeNull);

    tx.exec_prepared(myStatement, p);
juzzlin commented 3 years ago

Instead of passing an int, you can pass a std::optional<int>, or a std::unique_ptr<int>, or a std::shared_ptr<int>. If it contains no value (e.g. a default-constructed std::optional<int>), then that's an SQL null.

Plus, in the case of a char *, of course a null pointer also translates to an SQL null.

Wow that's cool, thanks! 😊

jtv commented 3 years ago

So glad you like it!

JadeMatrix commented 3 years ago

@jtv what happens when you pass a std::nullopt_t? Or does it always need some maybe-insertable type at compile time, even if at runtime the value is null-like? Sorry, haven't really kept up with v7.

Edit: meant nullopt but put nullptr, but I guess the question stands for both.

jtv commented 3 years ago

A nullptr_t is fine. As is nullopt_t, if I've got the name right.

On Sat, Jul 3, 2021, 01:52 Joseph Durel @.***> wrote:

@jtv https://github.com/jtv what happens when you pass a std::nullptr_t? Or does it always need some maybe-insertable type at compile time, even if at runtime the value is null-like? Sorry, haven't really kept up with v7.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jtv/libpqxx/issues/373#issuecomment-873307441, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQYDE3MSF52AXI2WMENLMLTVZGKJANCNFSM4RXCSOAA .