Knotx / knotx-fragments

Fragments map-reduce processing using Graph flows, supplier and assembler.
https://knotx.io
Apache License 2.0
3 stars 5 forks source link

HTTP Action body interpolation fails for GraphQL query #206

Closed tomaszmichalak closed 3 years ago

tomaszmichalak commented 3 years ago

Describe the bug HTTP Action fails when we try to interpolate the body (this is GraphQL query with variables)

To Reproduce Steps to reproduce the behaviour:

  1. Configure the action:
    {
    factory = action
    config.actions {
      products {
        factory = http
        config {
          httpMethod = POST
          endpointOptions {
            path = /graphql
            domain = magento
            port = 80
            additionalHeaders = { 
              content-type = application/json
            }
            body = """{"query":"query ($categories: [String!]!) {\n    products(filter: { category_id: { in: $categories } }) {    \n        total_count\n        items {      \n            name      \n            description {\n                html      \n            }      \n            image {\n                url\n                label\n            }\n            small_image {         \n                url\n                label\n            }\n        }\n    }\n}","variables":{"categories":["{config.categories}"]}}"""
            interpolateBody = true
          }
          responseOptions {
            predicates = [SC_SUCCESS, JSON]
          }
        }
    }
  2. Refresh the page.

Expected behavior {config.categories} is replaced with 2 when the fragment is configured such as <knotx:snippet categories="2" .../>

marcinus commented 3 years ago

This is due to PlaceholdersResolver logic which looks for opening and ending brackets in a greedy way. So { "key": "{config.value}" } gets recognized as a whole field and is not matched, instead of - what would be best - to match only the not nested group {config.value}. Next time please provide some more details - it will be easier to debug :)

marcinus commented 3 years ago

The issue seems to be a bit more complex - simple unit test for the case above passes.

What happens for the case above:

  1. following placeholders are found: in: $categories, \n html \n, \n url\n label\n, \n url\n label\n, config.categories
  2. all substitutors are queried. Only the last one matches the Fragment's configuration source
  3. the value gets substituted with value 2
  4. another search for unresolved placeholders is made. Now unresolved placeholders are: in: $categories, \n html \n, \n url\n label\n, \n url\n label\n, ⚠️ "categories":["2"]
  5. all unresolved placeholders get replaced with empty strings.

The mismatch is a nested result of a few factors:

The sequential substitution and the additional search are things we should eventually drop off. Otherwise, we might end up with substituting nested values, which would be indeterministic and not desired, e.g.: Query: {config.key} -> Query: SET {payload.otherKey} TO NULL -> Query: SET !unexpected! TO NULL.

Also, the substitution of not resolved placeholders to empty string should be configurable. I think it would be best if we just replaced placeholders that match any of used source definitions, otherwise end up with the problems above.