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

Deferred response headers should be handled more clearly from rhai #2936

Closed
garypen opened this issue Apr 12, 2023 · 2 comments · Fixed by #2945
Closed

Deferred response headers should be handled more clearly from rhai #2936

garypen opened this issue Apr 12, 2023 · 2 comments · Fixed by #2945
Assignees

Comments

@garypen
Copy link
Contributor

garypen commented Apr 12, 2023

We currently provide map_response() from rhai, but any deferred responses don't contain headers and so trying to access them will result in an error.

This is a difficult situation to deal with. We have a few options:

  1. Document this more clearly and rely on scripting to handle errors appropriately.
  2. Provide access to alternative mapping mechanisms (e.g.: map_first_graphql_response()) and document it's use.
  3. Modify the header accessing mechanisms to silently ignore operations on deferred responses.
  4. ... ?

I think 1 is ok for now. I think 2 is the correct answer. I don't like 3, but we could consider it.

@garypen garypen self-assigned this Apr 13, 2023
@garypen
Copy link
Contributor Author

garypen commented Apr 13, 2023

I have experimented with adding support for 2 and, to be honest, I don't think the DX is very good. I added map_first_response() support, which provided access to the headers, but you then have code that is calling map_first_response() and map_response() and you get overlap since both callbacks are invoked for the first response. So, then it becomes a question of should I add map_rest_response() and that becomes quite convoluted and feels like we are exposing too much complexity to the developer...

Perhaps 1 is the best answer. Just leave things as they are but clearly document that when handling responses, headers will not be present for deferred responses and that exceptions need to be handled.

@garypen
Copy link
Contributor Author

garypen commented Apr 14, 2023

The best way to address this is to improve the documentation and add the is_primary() method so that a script can determine at run time if a response is primary (i.e. has headers) or not.

@garypen garypen linked a pull request Apr 14, 2023 that will close this issue
5 tasks
garypen added a commit that referenced this issue Apr 20, 2023
Currently, if a rhai script errors in rhai, the rhai plugin ignores the
error and returns None in the stream of results.

This has two unfortunate aspects:
 - the error is not propagated to the client
 - the stream is terminated (silently)

The fix captures the error and propagates the response to the client.

Fixes #2935 #2936 

<!-- start metadata -->

**Checklist**

Complete the checklist (and note appropriate exceptions) before a final
PR is raised.

- [x] Changes are compatible[^1]
- [x] Documentation[^2] completed
- [x] Performance impact assessed and acceptable
- Tests added and passing[^3]
    - [x] Unit Tests
    ~- [ ] Integration Tests~
    - [x] Manual Tests

**Exceptions**

*Note any exceptions here*

**Notes**

[^1]. It may be appropriate to bring upcoming changes to the attention
of other (impacted) groups. Please endeavour to do this before seeking
PR approval. The mechanism for doing this will vary considerably, so use
your judgement as to how and when to do this.
[^2]. Configuration is an important part of many changes. Where
applicable please try to document configuration examples.
[^3]. Tick whichever testing boxes are applicable. If you are adding
Manual Tests:
- please document the manual testing (extensively) in the Exceptions.
- please raise a separate issue to automate the test and label it (or
ask for it to be labeled) as `manual test`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant