kong icon indicating copy to clipboard operation
kong copied to clipboard

feat (pdk) getters on service request

Open pamiel opened this issue 6 years ago • 3 comments

Summary

This PR is to add getter functions in the pdk.service.request object: get_method, get_scheme, get_host, get_port, get_path, get_raw_query, get_query_arg, get_query, get_header and get_headers.

get_raw_body and get_body are not in scope, so far.

During the review of this PR, could you please pay some specific attention to:

  • The check of the phase. I guessed that all getters should be accessible from PHASE.request
  • The getters for the host and port are based on ngx.ctx.balancer_data. Is it a correct assumption? (the tests tend to say ‘yes’, but I would like that you confirm this assumption)

To be honest, I made many copy/paste from pdk.request… but hopefully there are a couple of differences ;)

Note that making this exercice on getters, I realized that I have a concern related to the setters of the Service Request (and the impact of them on the Request). I’m about to post a topic on KongNation for a discussion on that (as this is somehow out of this PR).

Full changelog

  • Updated pdk.service.request
  • Implemented tests: I started by implementing tests under t/01-pdk/06-service-request/ and I realized at the end that many test cases were not possible or meaningless to implement with this solution (contrary to the tests on pdk.request) as it was just checking the value that was just being set through another way, without really checking that the right value was set at the right place by Kong routing mechanism itself. So I implemented another set of tests using busted. Tests are located in the spec/02-integration/08-pdk/01-service-request_spec.lua file (spec/02-integration/08-pdk/ is indeed a new directory). For this, I also implemented a new test plugin located in the spec/fixtures/custom_plugins/kong/plugins/pdk-service-request/ directory. Please, tell me if you prefer keeping only one of the test sets (I would say the one based on busted as this is the one with the most important test coverage), or if you want to keep both.

PDK update and tests are provided in 2 different commits.

Issues resolved

pamiel avatar Oct 09 '18 16:10 pamiel

Hi all, Is there any missing information for the review of this PR ? I'm at your disposal for any missing stuff: feel free to ask :P Thanks !

pamiel avatar Oct 30 '18 09:10 pamiel

Hi @pamiel,

Sorry we didn't get a chance to look at this for now. We are still busy with releasing Kong 1.0 these days. We will let get back to this when we have more time on our hands to review such large PRs. Thanks! :)

thibaultcha avatar Nov 08 '18 01:11 thibaultcha

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Apr 09 '20 06:04 CLAassistant

@pamiel Please re-open if this is still needed.

hbagdi avatar Oct 25 '22 21:10 hbagdi