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

Added ability to specify x-request-id header #6

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sivaschenko
Copy link
Member

Added ability to specify x-request-id header.

@chfabbro
Copy link
Member

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:

public static function generateCommonAPIHeaders(CoreConfig $config, string $access_token = null) : array

    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;
    }

@sivaschenko
Copy link
Member Author

sivaschenko commented May 29, 2019

@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.

@chfabbro
Copy link
Member

chfabbro commented May 29, 2019

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.)

@sivaschenko
Copy link
Member Author

sivaschenko commented May 29, 2019

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

@chfabbro
Copy link
Member

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.

@sivaschenko
Copy link
Member Author

@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.

@chfabbro
Copy link
Member

@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.

@sivaschenko
Copy link
Member Author

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

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.

3 participants