gsmainclusivetechlab / interop-test-platform

MIT License
6 stars 1 forks source link

Store Request/Response Templates as Strings #353

Closed cjol closed 3 years ago

cjol commented 3 years ago

Currently, I think request/response templates are parsed from the YAML file as structured objects, and stored in the DB as the same. My expectation was that we would store these templates/examples as strings so that we can have more control over the exact payload that the simulators send. Here are some example payloads that I think we can't achieve with the current setup:

Inject a numeric value

Input:

{ 
  "first": 1, "second": 2, "third": {{ 1 + 2 }} 
}

Expected Output:

{ 
  "first": 1, "second": 2, "third": 3 
}

Problem: Testcase will fail to parse because {{ 1 + 2 }} is not valid JSON.

Inject a non-primitive value

Input:

{ 
  "status": "received", 
  "body": {{ test_steps[0].request.body }} 
}

Expected Output:

{ 
  "status": "received", 
  "body": { "amount": 100, "currency": "GBP" }
}

Problem: Testcase will fail to parse because {{ test_steps[0].request.body }} is not valid JSON.

Inject a variable into key position

Input:

{ 
  "{{ environment.RESULT_KEY }}": "accepted" 
}

Expected Output:

{ 
  "my_custom_result": "accepted" 
}

Problem: JSON keys are not currently examined for variable substitution, so the output will contain {{ environment.RESULT_KEY }}

Create empty request body

Input:

Expected Output:

Problem: Testcase will fail to parse because "" is not valid JSON (and cannot be created through a visual JSON editor)

Create malformed JSON (e.g. to test graceful response to errors)

Input:

{ comment: "no quote marks around the comment field, and there is no closing brace"

Expected Output:

{ comment: "no quote marks around the comment field, and there is no closing brace"

Problem: Testcase will fail to parse because the document is not valid JSON (and cannot be created through a visual JSON editor)

Create non-JSON payload (e.g. an XML response for a future API)

The problem here is that we won't be able to parse the testcase when we import it (or create it through the JSON editor), if the template is expecting a valid JSON object. Input:

<Status>Accepted</Status>

Expected Output:

<Status>Accepted</Status>

Problem: Testcase will fail to parse because the document is not valid JSON (and cannot be created through a visual JSON editor)

cjol commented 3 years ago

N.B. if the template is stored as a string, we cannot manipulate it very easily (e.g. to retrieve body.foo.bar inside the JSON document). In places where we need to do such manipulation, I suggest that we parse the JSON document on-demand (after applying twig substitutions) instead of "pre-parsing" it and storing the structured object in the database.

JSON is the expected format for most of our payloads, so it makes sense that we have prioritised it, but we should not force the payloads to always be JSON (e.g. in the common case of an empty response).

I think the visual JSON editor as part of the test case editor is nice, but it is less important to me than supporting the patterns above. So if that means that it is necessary to remove the JSON editor, then I'm happy to stick with a plaintext textarea.

agalaguz commented 3 years ago

Hi Christopher, @cjol

Just wanted to check if our previous work on #343 still makes sense. Variables support branch contains the next features:

1. Inject a numeric value

2. Inject a non-primitive value

Notes to 1 and 2:


3. Inject a variable into key position:

It's not there, but should be possible to be developed pretty fast (e.g. <1d)

4. Create empty request body:

It's not there, but should be possible to be developed pretty fast (e.g. ~1-2d). Maybe we are to add some syntax for YAML and for the existing TC edit UI

Notes to 3 and 4:

Looks like both options can be added pretty quickly w/o global changes to the existing backend and UI.


5. Create malformed JSON (e.g. to test graceful response to errors):

Not there and need to think about it as it requires pretty big changes to the way of data storing (now we store JSONs in db).

6. Create non-JSON payload (e.g. an XML response for a future API):

Same as 5.

Notes to 5 and 6:

Seems like, from the technical perspective, 'not valid JSON' and 'non-JSON payload' are almost the same - i.e. require adding some way to handle non-JSON data, as Postman does. We need to investigate how much work is it, but it looks like a bit slice of existing code to be changed... These features are to be used in request.body and response.body only, right?


Suggestions:

  1. If the way items 1-2 works is fine, items 3-4 development timeline is acceptable, and delay for items 5-6 is ok - we may deliver the next version of Variables early next week.
  2. If the actual state of Variables support and our suggestion above is not sufficient, please let us know and we'll try to find another solution.

Please let us know if this works. Thank you!

cjol commented 3 years ago

Hi Anton,

I'm a little bit disappointed because I did anticipate these problems - but it looks like I didn't get my concerns across at the right time. Quoting from #343:

  1. Define one big template string for the request body in the YAML
  2. Store the template as a string in the database

In terms of moving forward, there are still some oustanding practical problems with 1-2, which I will add detail to below. 4 is also a practical problem, but we can add a workaround as you describe it. 3, 5 & 6 are lower in importance because they are "theoretical" problems that we can avoid in our current test case roadmap.

I am a little bit surprised that supporting non-JSON payloads is a big change because I didn't think we did any processing to the payload (I thought we just read it from the test case and dump it into the request). But you all know the codebase better than I do, so I will defer to your judgement!

For now, let's add a workaround to allow us to construct empty bodies (4), and I can tolerate the remaining problems for now. I would also be thankful if you can find some time to do the investigation of work required for supporting non-JSON payloads so that I can think about when to include it in our roadmap.

Detail on outstanding problems > 1. Inject a numeric value > `{ "first": 1, "second": 2, "third": "{{ 1 + 2 }}"}` This is not actually injecting a numeric value, - the result will be `"third": "3"` instead of `"third": 3`. Note that the result contains quote marks that are undesirable. I think for the current testcases, we can work around this issue but we will need to fix it eventually. > `{ "body": {{ test_steps[0].request.body }} }` is not supported as right now we can't get objects/arrays using Twig. This is actually specifically what I meant with the phrase "non-primitive" values. I'm not sure this is a Twig problem because we can get an object in Twig and [convert it](https://twig.symfony.com/doc/1.x/filters/json_encode.html) into a JSON string: `{ "body": {{ test_steps[0].request.body | json_encode() }} }`. The problem is actually the same as the numeric value - if we inject the JSON string using the current system, there will be undesirable quote marks around the result. It's a common pattern in the MM API to "repeat" a payload from a previous message as part of the confirmation. Again, I think we can work around this issue for the current testcases by writing out the individual values inside the object one-by-one.
cjol commented 3 years ago

Another note: it looks like we are preserving the old syntax for injecting environment variables and the new syntax (amount: '{{ ${FEE} + ${AMOUNT} + ${fee} + ${amount} }}'). That seems redundant - we can simplify things by only supporting the new syntax:

amount: '{{ env.FEE + env.AMOUNT + env.fee + env.amount }}'`

(N.B. the environment variables are strings but we need numeric data - luckily Twig will automatically convert the data type for us when we're doing maths)