stock-api-libphp icon indicating copy to clipboard operation
stock-api-libphp copied to clipboard

Added ability to specify x-request-id header

Open sivaschenko opened this issue 5 years ago • 8 comments

Added ability to specify x-request-id header.

sivaschenko avatar May 29 '19 14:05 sivaschenko

There are a lot of code changes here for one new header. Can you summarize all the changes and how you implemented this? Some of the changes are only formatting. Also, I sense this is more work than is needed. There is already support for request-id, but it is always static. It seems you could add an optional parameter in this line, and therefore not need all the other changes: https://github.com/adobe/stock-api-libphp/blob/99a6786ca6b92cebab1f274be7a4a765250a59d7/src/Utils/APIUtils.php#L36

    public static function generateCommonAPIHeaders(CoreConfig $config, string $access_token = null) : array
    {
        $request_id = static::getUUID();
        
        if ($access_token !== null) {
            $access_token = 'Bearer ' . $access_token;
        }
        
        $headers = [
            'headers' => [
                'x-api-key' => $config->getApiKey(),
                'x-product' => $config->getProduct(),
                'Authorization' => $access_token,
                'x-request-id' => $request_id,
            ],
        ];
        return $headers;
    }

chfabbro avatar May 29 '19 16:05 chfabbro

@chfabbro thanks for the review!

To summarize:

  • I've added property and methods setRequestId and getRequestId to all the request classes (extracting the requestId and locale field to an abstract class to avoid code duplication).

  • I've added overriding of common headers retrieved from APIUtils::generateCommonAPIHeaders to set the x-request-id header in all the Client classes. (I wasn't able to find a good and backward compatible approach to avoid code duplication in this case)

     if (!empty($request->getRequestId())) {
         $headers['headers']['x-request-id'] = $request->getRequestId();
     }
    
  • Code formatting was done automatically by the IDE - unnecessary spaces removed from empty lines - let me know if you'd like this formatting to be removed from the pull request.

I didn't add an additional parameter to generateCommonAPIHeaders as I thought this method responsibility is to provide default headers (conclusion based on method name), also if that parameter will be added - all client classes will need to be updated anyway to pass the parameter.

Let me know if I should add an additional optional $requestIdparameter to generateCommonAPIHeaders to avoid adding of if conditions to each client class.

sivaschenko avatar May 29 '19 18:05 sivaschenko

Hmm... I get multiple errors when I try to run the tests. Were you able to run tests, or were you not able to install astock/phpcs-psr2-stock? Unfortunately that is an internal library--yet another reason why Adobe is not good at open source! (Note that there is no automation with public Github and our repos, so no tests are run. You must run them yourself before submitting. Our internal Adobe git repo has integrated testing, but that is private to Adobe.)

chfabbro avatar May 29 '19 21:05 chfabbro

@chfabbro if you confirm the changes, I will adjust the tests and add test coverage for the introduced functionality.

sivaschenko avatar May 29 '19 21:05 sivaschenko

I will need to wait for @slaanesh to review. He is the final approver of any changes to public Git, and created the rules for how submissions get approved. He is also much more familiar with PHP and code style than I am.

chfabbro avatar May 29 '19 21:05 chfabbro

@chfabbro tests are fixed and new functionality test coverage is provided.

I did also fix the AdobeStock\Api\Test\LicenseFactoryTest::downloadAssetRequestShouldReturnValidRequestObject that is failing on master branch.

sivaschenko avatar May 30 '19 13:05 sivaschenko

@sivaschenko Thanks! I sent you an offline Slack message. I will get a dev resource to help me review both of your PRs. I will also add you to the internal JIRA tickets I am creating for these.

chfabbro avatar May 31 '19 19:05 chfabbro

Hi @chfabbro the formatting changes are removed from the pull request

sivaschenko avatar Oct 01 '19 19:10 sivaschenko