superfaceai / one-sdk-js

1️⃣ One Node.js SDK for all the APIs you want to integrate with
https://superface.ai
MIT License
47 stars 3 forks source link

[BUG] Not resolving API keys #287

Closed martinalbert closed 2 years ago

martinalbert commented 2 years ago

Expected Behavior

Should resolve API keys received from super.json (such as $PROVIDER_API_KEY) to key loaded from .env in new flow of testing library.

Current Behavior

Found this when testing new sdk with station in this PR. Currently it uses not resolved api key from super.json and therefore nock is not able to match the request correctly.

Possible Solution

Resolve credential when it starts with $.

Steps to Reproduce

  1. Get into #279 in station
  2. yarn install
  3. run DEBUG=nock* yarn test communication/send-email/maps/mandrillapp.test.ts to see nock matching

Your Environment

Include as many relevant details about the environment you experienced the bug in. Preferably include:

Jakub-Vacek commented 2 years ago

Could you please try to replicate in fresh project using whole sdk (like user would )? I tried it with "in code" super json and it resolves env value correctly.

Jakub-Vacek commented 2 years ago
const { SuperfaceClient } = require('@superfaceai/one-sdk');

const sdk = new SuperfaceClient(
  {
    superJson: {
      "profiles": {
        "address/ip-geolocation": {
          "version": "1.0.1",
          "providers": {
            "ipbase": {
            }
          }
        }
      },
      "providers": {
        "ipbase": {
          "security": [
            {
              "id": "apikey",
              "apikey": "$API_KEY"
            }
          ]
        },
      }
    }
  }
);

async function run() {
  const profile = await sdk.getProfile('address/ip-geolocation');

  const provider = await sdk.getProvider('ipbase')

  // Use the profile
  const result = await profile
    .getUseCase('IpGeolocation')
    .perform({
    }, {
      provider,
    }

    );

  console.log(result.unwrap());
}

run();
martinalbert commented 2 years ago

This is only happening with new testing flow in testing library - @superfaceai/testing@3.0.0-beta.1

martinalbert commented 2 years ago

Here is minimal reproduction of bug with address/geocoding/here integration from station: testing-new-sdk.zip

  1. download and unzip
  2. yarn install
  3. yarn test (DEBUG=nock* yarn test for more detail about request matching)
Jakub-Vacek commented 2 years ago

@martinalbert So is this bug in SDK or in testing lib? I wasn't able to replicate it with using SDK directly so I think it is in testing lib. If bug is in testing lib we should close this issue to not confuse users.

martinalbert commented 2 years ago

Well this depends on whether intended behaviour is that BoundProfileProvider should resolve credentials or not. If not, I'll move it to testing library and open a PR for the fix.

Jakub-Vacek commented 2 years ago

BoundProfileProvider should resolve credentials if it would take super.json as input but it is not using super.json. It takes SecurityConfiguration (which contains resolved credentials). It is aligned with idea that BoundProfileProvider just takes prepared stuff (asts, provider.json, config, security) and runs it. I think this definition allows us to use it in testing-lib and one-service.

Personally I think this is fault (probably mine) in migrating to BoundProfileProvider usage in testing lib not bug in SDK.

@lukas-valenta @freaz what do you think?

freaz commented 2 years ago

I agree with @Jakub-Vacek that BoundProfileProvider shouldn't resolve credentials, and it should be passed in. It allows way more versatility how BoundProfileProvider can be used.

martinalbert commented 2 years ago

Ok, I'm closing this and will open PR in testing repo.