Support a mix of locator and creation configs for SNS

[kibertoad]

please review carefully, this was coded in a late night session using the scraps of the mental capacity, likely to be ugly and have gaps.

would be great to clean up the logic and split the code in the follow up prs.

implementation rushed to unblock Billinq service :-/

[kamilwylegala]

TS is complaining now that initSnsSqs().queueUrl is string|undefined.

[kamilwylegala]

@kibertoad I solved this by adding if to check if queue url is present. From the code it looks it should be mandatory parameter if subscriptionArn is provided, we must provide queueUrl too. What do you think?

[kibertoad]

isn't there branching after the check you added, where we skip the queueUrl-related logic if it's not present?

we should support passing queueName instead of queueUrl, no? or we always resolve url based on name earlier?

[kamilwylegala]

There is branch with check, but it didn't guarantee that queueUrl is present after that:

  let queueName: string
  if ((locatorConfig as SQSQueueLocatorType).queueUrl) {
    const splitUrl = (locatorConfig as SQSQueueLocatorType).queueUrl.split('/')
    queueName = splitUrl[splitUrl.length - 1]
  } else {
    queueName = creationConfig!.queue.QueueName!
  }

  return {
    subscriptionArn: locatorConfig.subscriptionArn,
    topicArn: subscriptionTopicArn,
    queueUrl: locatorConfig.queueUrl,
    queueName,
  }

From this code it looks that queueUrl is mandatory.

[kibertoad]

I don't see it. if queueUrl is not present here, we will just return queueUrl: undefined

[kamilwylegala]

Yep, but queueUrl cannot be null. It's required in AbstractSnsSqsConsumer, see line: 104

[kamilwylegala]

How configuration in billing service could look like?

Arguments passed to constructor should be wrapped with isTest() e.g.:

{
  "creationConfig": isTest() ? { /* legit creation config */ } : undefined, //In production creation of topics will be disabled.
  "locatorConfig": isTest() ? undefined : { /* legit locate config */ } //In production we will do read-only
}

[kibertoad]

I'd say that creationConfig would always have an entry for the queue, and conditionally will include entry for the topic.
locatorConfig will conditionally either have an entry for the topic, or be empty

[kamilwylegala]

Let me visualize it with the test. I think the rest of the comments will clarify too.

[kamilwylegala]

Why does it mix the configuration of both: locatorConfig and creationConfig? Both can be provided and we return queueName from creation config but queue url comes from locate config.

[kibertoad]

I think we only follow this branch in case we ended up using locate config, but I might be wrong, worth checking

[kamilwylegala]

Couldn't we rework this method and introduce a breaking change?

[kibertoad]

potentially. if you see a clear way to do this that would simplify the code, go for it

[kamilwylegala]

Is it needed? It looks like new API not related to support both creation/locate configs.

[kibertoad]

it is needed to support both having a predefined arn and a lookup by topic name, which new logic relies upon

[kamilwylegala]

OK, so precedence will always be:

• use ARN if present
• then resolve by name (async lookup)

[kamilwylegala]

EOR1