search

shabiel / M-Web-Server

A YottaDB and Caché compatible HTTP server
Apache License 2.0
24 stars 19 forks source link

CORS support and enhancements #35

Closed sumeetchhetri closed 5 years ago

sumeetchhetri commented 5 years ago

Set server port value in ^%webhttp Add support for CORS configuration (Refer - https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#The_HTTP_response_headers) Set cors configuration in ^%webhttp Add Server execution parameters (Additional custom hooks)

  1. Pre Execution Function - Routine to be invoked before processing each request (Imagine an authentication token handler which returns 401 if token is invalid or not present in the request)
  2. First Argument Response/Request - Order of arguments to be passed to request processing routine (should we pass response as first argument or request)
  3. Auto-serialize response as JSON - Should we auto-serialize response (assuming it is a well defined JSON object), providing support for the special HTTPRSP("_http") value in-case we need to return headers or status codes along-with the JSON object
sumeetchhetri commented 5 years ago

This is a Work In Progress, @shabiel can you please review it once and provide feedback so I can consider the same, before finalizing/validating all my changes

shabiel commented 5 years ago

Okay. I will get back to you on Monday.

We already have CORS support. It was merged in 2 or 3 weeks ago I think.

--Sam

On Fri, Aug 23, 2019 at 10:01 AM Sumeet Chhetri notifications@github.com wrote:

This is a Work In Progress, @shabiel can you please review it once and provide feedback so I can consider the same, before finalizing/validating all my changes

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

sumeetchhetri commented 5 years ago

CORS should always be optional for security reasons and we should be able to configure all the required CORS headers as required. So currently the support is incomplete, moreover I have fixed one performance issue while breaking down post/put arguments encoded as form params. Additionally I have added support for a hook/flags to customize the request invocation/handling phase.

On Fri, Aug 23, 2019, 7:39 PM Sam Habiel notifications@github.com wrote:

Okay. I will get back to you on Monday.

We already have CORS support. It was merged in 2 or 3 weeks ago I think.

--Sam

On Fri, Aug 23, 2019 at 10:01 AM Sumeet Chhetri notifications@github.com wrote:

This is a Work In Progress, @shabiel can you please review it once and provide feedback so I can consider the same, before finalizing/validating all my changes

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/shabiel/M-Web-Server/pull/35?email_source=notifications&email_token=AAMUHGJEJKF5WMT2MLXIY2LQF7VP3A5CNFSM4IO76AYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5AKFVA#issuecomment-524329684, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMUHGNLC2CXNCWQCKJROU3QF7VP3ANCNFSM4IO76AYA .

sumeetchhetri commented 5 years ago

@shabiel, did you get a chance to review this, I have already validated and tested all the changes and made necessary code changes required.

shabiel commented 5 years ago

Did you see this message I sent you?

After some thought, I decided I cannot accept any part of this pull request, as it's too big, and you introduce bugs in the code ($SY change).

Here's what I want from you in order to accept your changes.

  1. Only one pull request per feature. Don't put all the changes all together.
  2. Write Unit Tests for all new features.
  3. Don't modify code that is currently working (like $SY) without creating an issue on Github first so we can discuss it first.

You should also include real life examples of what you are trying to solve. Of your entire pull request, I only see utility for the CORS change. The rest are features I don't particularly want; but will consider them if you provide a real world problem they are trying to solve.

On Wed, Aug 28, 2019 at 5:36 AM Sumeet Chhetri notifications@github.com wrote:

@shabiel https://github.com/shabiel, did you get a chance to review this, I have already validated and tested all the changes and made necessary code changes required.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/shabiel/M-Web-Server/pull/35?email_source=notifications&email_token=AAIS3QQRHXOQPJMVPIU5OEDQGZBKTA5CNFSM4IO76AYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5KQHJQ#issuecomment-525665190, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIS3QRJY5ZQHKMGFZAPPYDQGZBKTANCNFSM4IO76AYA .

sumeetchhetri commented 5 years ago

Sure let me come with separate Pull Requests then