Implement QueryBuilder

[ken-matsui]

I created new classes rather than editing Mapper since the design is completely different and non-intrusive. In my opinion, the current Mapper has a little complex user interface. However, if you do not like the implementation design and direction, I close this PR.

By using this patch, we can write a query more beautifully:

https://github.com/poacpm/api.poac.pm/blob/c01da77374e212ebf035f22dce76086e5ed52eeb/controllers/v1.cc#L30-L38

Conditionally adding filters will also be much easier:

https://github.com/poacpm/api.poac.pm/blob/c0b02ce3377de764623a5f1c7618e202fa152fd4/controllers/v1.cc#L97-L105

The execSync method returns Row or Result when SelectAll is false because selecting partial columns is not supported by the auto-generated drogon model (reported Invalid SQL result for this model). By improving the template for drogon model, we will be able to return simply T or std::vector<T>.

Although, for now, this only supports select and a few filters, I will implement all queries and filters and add a document to use QueryBuilder and tests. Additionally, I would like to replace Mapper completely in the future, but what do you think?

[an-tao]

@ken-matsui Thanks so much for the PR. It LGTM. would u please add the execAsync and execCoro method to the FilterBuilder class?

[ken-matsui]

@an-tao

Thanks so much for the PR. It LGTM.

Thank you for your review!

would u please add the execAsync and execCoro method to the FilterBuilder class?

Sure!

[an-tao]

@an-tao

Thanks so much for the PR. It LGTM.

Thank you for your review!

would u please add the execAsync and execCoro method to the FilterBuilder class?

Sure!

This is a big feature, I think we need more reviewers :)

[ken-matsui]

It seems GitHub Actions on 092f40f571fe4e9ed14a3d4894413880fed20979 and e3e8c6973e6ee122d689e59fe3a5ef33248fa115 should be reran.

[an-tao]

@ken-matsui would you please add some tests to the db_test.cc for this patch? thanks.

[ken-matsui]

I do not have a compiler that implements coroutines, so the tests may fail.

[ken-matsui]

Hi @an-tao,

I've added tests for the builders. However, I am not sure they are working correctly because they are frequently skipped somehow.

Let me ask one question about security. How are you preventing SQL injection? Or does DbClient assure it? If needed, I think I have to implement protection for the builders.

[ken-matsui]

Also, do you have any plan to drop the support of C++14? Using C++17 or over would clean up our codes. For example, cpr recently decided to set the minimum required C++ version to C++17.

https://github.com/libcpr/cpr/releases/tag/1.9.0

[an-tao]

Hi @an-tao,

I've added tests for the builders. However, I am not sure they are working correctly because they are frequently skipped somehow.

Let me ask one question about security. How are you preventing SQL injection? Or does DbClient assure it? If needed, I think I have to implement protection for the builders.

If you use placeholders for parameters, DbClient can prevent SQL injection.

[ken-matsui]

@an-tao

If you use placeholders for parameters, DbClient can prevent SQL injection.

Thank you for your answer. Do you prefer to include the change to use placeholders in this PR?

[an-tao]

@an-tao

If you use placeholders for parameters, DbClient can prevent SQL injection.

Thank you for your answer. Do you prefer to include the change to use placeholders in this PR?

Yes. thanks

[ken-matsui]

@an-tao

I've implemented the builder to use placeholders. Please let me know if you need additional implementation.

[ken-matsui]

@an-tao Thank you for your review! I've fixed both as reviewed.

[an-tao]

@an-tao Thank you for your review! I've fixed both as reviewed.

Thanks, please rebase on the new master branch.

[ken-matsui]

@an-tao

Could you please check why the bad-free error occurs? https://github.com/drogonframework/drogon/runs/7778696421?check_suite_focus=true#step:15:46755

[an-tao]

@an-tao

Could you please check why the bad-free error occurs? https://github.com/drogonframework/drogon/runs/7778696421?check_suite_focus=true#step:15:46755

I can't say where is the problem with this error, maybe we could try to use g++11 because the g++10 has some bugs with coroutines.

[an-tao]

@ken-matsui please rebase on the master branch, thanks.

[ken-matsui]

Through debugging on my Ubuntu machine, I found two things when I executed ./test.sh -t.

1. Building with -DCMAKE_CXX_FLAGS="-fcoroutines" causes this error; otherwise, nothing was caused (Everything is ok! was printed out)
2. With -DCMAKE_BUILD_TYPE=Debug, the following error appeared. (EDIT: Fixed on ba9c1f549c9586dda4184b4d67f9657a39244c81, now regardless of build types, same as 3)
3. (Release will emit the same error with CI)

$ ./test.sh -t
...

In test case PostgreTest
↳ /home/kmatsui/Desktop/drogon/orm_lib/tests/db_test.cc:957  PASSED:
  MANDATE(result.size() != 0)

db_test: /home/kmatsui/Desktop/drogon/lib/inc/drogon/utils/coroutine.h:510: const T& drogon::CallbackAwaiter<T>::await_resume() const [with T = std::vector<drogon_model::postgres::Users, std::allocator<drogon_model::postgres::Users> >]: Assertion `result_.has_value() == true || exception_ != nullptr' failed.
./test.sh: line 203: 2931751 Aborted                 (core dumped) ./orm_lib/tests/db_test -s
Error in testing

Environment

$ uname -sp
Linux x86_64

$ g++ --version
g++ (Ubuntu 11.2.0-19ubuntu1) 11.2.0
...

$ git branch --show-current
implement-builders

[ken-matsui]

~So, my execCoro implementation is something wrong since the assertion failure is emitted as the following line is executed.~

~https://github.com/ken-matsui/drogon/blob/f0a88568a89c852322b5ecd87c6be9310b6c1b3c/orm_lib/tests/db_test.cc#L970-L973~

~I'll have a look into it.~

---

EDIT: The assertion failure was fixed on ba9c1f549c9586dda4184b4d67f9657a39244c81.

[ken-matsui]

@an-tao To make things simpler with this PR, I propose removing support for coroutines and addressing it in a separate PR. What do you think?

[ken-matsui]

@an-tao Do you think this PR is ready to be merged?

[an-tao]

@ken-matsui Would you please add a section to the wiki-doc for this patch? thanks so much.