Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to use path params and body in a single query? #8

Open
php-coder opened this issue Jul 15, 2020 · 7 comments
Open

How to use path params and body in a single query? #8

php-coder opened this issue Jul 15, 2020 · 7 comments
Labels
decision Requires making a decision

Comments

@php-coder
Copy link
Owner

php-coder commented Jul 15, 2020

In the current approach, we pass parameters to SQL query only from a single source (path params or body params):

  • GET: uses path params
  • POST: uses body params
  • PUT: uses body params
  • DELETE: uses path params

But even for a simple PUT endpoint it isn't enough: we want to extract object ID from a path and other parameters from a body. Here is an example of generated code that we should improve:

app.put('/v1/categories/:categoryId', (req, res) => {
  pool.query(
    'UPDATE categories SET name = :name , name_ru = :nameRu , slug = :slug , updated_at = NOW() , updated_by = :userId WHERE id = :categoryId',
    { "name": req.body.name, "nameRu": req.body.nameRu, "slug": req.body.slug, "userId": req.body.userId, "categoryId": req.body.categoryId },

We can see that categoryId is bound to req.body.categoryId while it should use a value from req.params.categoryId.

Also there will be more cases where we need to combine different sources: userId might be extracted from a header. We might want to access cookies or query parameters.

@php-coder php-coder added the decision Requires making a decision label Jul 15, 2020
@php-coder
Copy link
Owner Author

Option 1: improve the current logic

Instead of always binding to body parameters, we can first search what path parameters are defined. In the example above, categoryId is defined in a path, so this can take precedence. Everything else will be bind to default source.

Pros:

  • minimal modifications are required

Cons:

  • the more conventions (magic) we have, the more knowledge is required from users to grasp it

@php-coder
Copy link
Owner Author

php-coder commented Jul 15, 2020

Option 2: enhance SQL queries with variables

Example:

- path: /v1/categories/:categoryId
  put: >-
    UPDATE categories
       SET name = #{body.name}
         , name_ru = #{body.nameRu}       <-- NEW
         , slug = #{body.slug}
         , updated_at = NOW()
         , updated_by = #{header.userId}  <-- NEW
     WHERE id = #{path.categoryId}        <-- NEW

The syntax can be different:

  • $body.name
  • ${body.name}
  • #{body.name}

body is one of the sources and can be header, path, query, cookie, etc

Pros:

  • this solves the issue and cover more cases because it adds a powerful capabilities

Cons:

  • we step away from a plain SQL and introduce SQL-on-steroids
  • user has to know our conventions
  • queries get verbose

@php-coder
Copy link
Owner Author

php-coder commented Jul 15, 2020

Option 3: mix the previous approaches

Example:

- path: /v1/categories/:categoryId
  put: >-
    UPDATE categories
       SET name = :name
         , name_ru = :nameRu
         , slug = :slug
         , updated_at = NOW()
         , updated_by = #{header.userId}  <-- NEW
     WHERE id = #{path.categoryId}        <-- NEW

Pros:

  • SQL queries will be less cryptic because parameters will have a default source and user has to specify a source explicitly only for edge cases

Cons:

  • user should keep in mind even more conventions (the first is about default source, the second is about overriding and its syntax)

@theshamuel
Copy link

theshamuel commented Jul 17, 2020

Hey @php-coder, to be honest the all cons about all options are make sense. So, if I were you, I'd look at another way:
modify a bit your yaml declaration. For example, I'd add a new directive params:

- path: /v1/categories/:categoryId
  params:
    - header: userId <--list of params that we take from header>
    - body: name, name_ru, slug <-- <list of params that we take from body>
 .....
  put: >-
    UPDATE categories
       SET name = :name
         , name_ru = :nameRu
         , slug = :slug
         , updated_at = NOW()
         , updated_by = :userId
     WHERE id = :categoryId

Pros:

  • do not need to modify plain SQL
  • this is cover your issue, I suppose

Cons:

  • user has to know our YAML conventions

@php-coder
Copy link
Owner Author

@theshamuel In other words, we can move binding logic into YAML configuration. Another con of that approach is that it makes configuration more verbose.

Thank you for the input! I'll think about it.

@php-coder
Copy link
Owner Author

For example, I'd add a new directive params:

- path: /v1/categories/:categoryId
 params:
   - header: userId <--list of params that we take from header>
   - body: name, name_ru, slug <-- <list of params that we take from body>
.....
 put: >-

By the way, this way we will make params global, i.e. it will be available within put and other possible verbs (get, post, etc). This is isn't desirable.

At the same time, if we try to move this into a method, we will have to introduce a property for a query:

- path: /v1/categories/:categoryId
  put:
    query: >-
      UPDATE categories
         SET name = :name
           , name_ru = :nameRu
           , slug = :slug
           , updated_at = NOW()
           , updated_by = :userId
     WHERE id = :categoryId
    params:
      - header: [ 'userId' ]
      - body: [ 'name', 'name_ru', 'slug' ]

php-coder added a commit that referenced this issue Jul 26, 2020
…citly bind to a desired origin

This feature allows us to use parameters from different sources. See example for testing PUT methods
in the tests.

BREAKING CHANGE: every :parameter should be revised and modified to be :b.parameter to bind to
request body or :p.parameter to bind to path parameter.

See also #8
@php-coder
Copy link
Owner Author

In order to push this forward, in the ddb2e16 commit the 2nd approach was partially implemented. All parameters now must have a prefix b or p: :b.param or p.param.

This might be not a best API but at least it allow us to proceed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
decision Requires making a decision
Projects
None yet
Development

No branches or pull requests

2 participants