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

Handle custom header additions without requiring "Upstash-Forward-*" as a prefix. #55

Merged
merged 10 commits into from
Nov 21, 2023

Conversation

msywulak
Copy link
Contributor

@msywulak msywulak commented Sep 15, 2023

Description:

Currently, if you need to send custom headers to a schedule's destination you must prefix them manually with "Upstash-Forward-*". If you forget to add the prefix the headers will be ignored.

Also, deletion did not work until using new parseResponseAsJson property.

Updates

  • Handle headers added with or without "Upstash-Forward" while ignoring set property headers.
  • Add parseResponseAsJson: false, to schedule deletion.

Fixes #52

@msywulak
Copy link
Contributor Author

Also, if you're alright with the method here or have a cleaner way to do it I could add it to the following as well:

  • topics.ts
  • client.ts (publish and publishJSON)

Probably could add in parseResponseAsJson: false, into a couple of places that need it now.

@ogzhanolguncu
Copy link
Contributor

Looks good to me. Since we added failure callback recently this ignoredHeaders[] requires an update. Also let's turn this into some sort of utility since we are going to call this in multiple places. And, finally please add an appropriate example similar to nodejsexample in the example/nodejs repo.

@msywulak
Copy link
Contributor Author

@ogzhanolguncu I haven't made an example yet but is this what you had in mind?

@ogzhanolguncu
Copy link
Contributor

ogzhanolguncu commented Nov 17, 2023

Util is looking good. What I have in my mind is NodeJS Example adding an another example here where you don't need pass Upstash-Forward-*. Just to showcase people its possible.

Maybe add another file here where you pass optinal headers to your callback.

@ogzhanolguncu
Copy link
Contributor

Also, if you're alright with the method here or have a cleaner way to do it I could add it to the following as well:

  • topics.ts
  • client.ts (publish and publishJSON)

Probably could add in parseResponseAsJson: false, into a couple of places that need it now.

And, let's do this since we now have a easier way to handle this with thanks toprefixHeaders func.

@msywulak
Copy link
Contributor Author

About to add the others but I was looking through the API docs and couldn't find a place where you can add custom headers on topics. I tested a couple of ways but couldn't make it work - trying to see if it was just undocumented but that doesn't look to be the case.

@ogzhanolguncu
Copy link
Contributor

The PR is looking good right now. However, I have a concern: what if someone adds a new default header to QStash, and we overlook it or forget to include it here? Since it won't be in the predefined list, it could result in rendering the default header as null, potentially causing issues on the servers.

@msywulak
Copy link
Contributor Author

At the end of the day the client is setting those headers after the util runs, if it was set on the original req, but changed the util up a bit to always ignore any header starting with "upstash-" or "content-type". This would make it so you could never set a Upstash specific header using the headers property, but I think that would likely be expected based on the comment for the header property saying that "these headers will be sent to your destination".

@ogzhanolguncu
Copy link
Contributor

ogzhanolguncu commented Nov 20, 2023

This is looking much better. I'll test it myself too and merge it tomorrow. Good work 👍🏻

@ogzhanolguncu ogzhanolguncu merged commit 64573cd into upstash:main Nov 21, 2023
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deleting Schedules Error: Unexpected end of JSON input
2 participants