Feat(Site Launch): Mock calls to Amplify

[kishore03109]

Problem

During development, the CreateDomainAssociation calls are rate-limited to 10 per hour. This low rate limit can cause issues during actual site launches that may affect operations.

Solution

To avoid impeding with the actual infrastructure rate limits, this PR mocks all calls to CreateDomainAssociation and GetDomainAssociation in LaunchesService. Only staging, vapt, and prod environments should be making actual calls to Amplify. Additionally, since we are mocking Amplify calls now, we no longer need to wait for 90 seconds after creating domain associations for Amplify to generate the required values.

The mocked objects may seem verbose, but they closely resemble the actual values received when calling Amplify.

Breaking Changes

There are no breaking changes.

Reviewer Notes

I am concerned that future devs working around site launch feature might be confused why they are not able to create domain associations during development. Not too sure if there is a way to highlight clearly in code that the calls are mocks apart from the proposed solution of already naming them mockCreateDomainAssociationOutput and mockCreateDomainAssociationOutput in LaunchesClient. Open to feedback on this!

Tests

To mock form responses, you can call the following code directly from server.js:

const formResponses = [
  {
    submissionId: "",
    requesterEmail: "[email protected]",
    repoName: "kishore-test",
    primaryDomain: "kishorewithwww.isomer.gov.sg",
    redirectionDomain: "www.kishorewithwww.isomer.gov.sg",
    agencyEmail: "[email protected]",
  },
  {
    submissionId: "",
    requesterEmail: "[email protected]",
    repoName: "test-vincent",
    primaryDomain: "kishorewithoutwww.isomer.gov.sg",
    redirectionDomain: "",
    agencyEmail: "[email protected]",
  },
]

formsgSiteLaunchRouter.handleSiteLaunchResults(formResponses, "test")

After mocking a site launch form submission, the launches and redirections in the database are populated, and emails are sent as expected.

Deploy Notes

New environment variables:

• MOCK_AMPLIFY_CREATE_DOMAIN_CALLS : mocking amplify calls so as to not impede with operations

Values in 1pw prod and staging env vars are also updated. TLDR; MOCK_AMPLIFY_CREATE_DOMAIN_CALLS is true in prod and staging env, false in local dev.

[harishv7]

This is going to block the main thread for 90s right?

Seems like "todo: push this check into a queue-like system when integrating this with cms" is crucial for integration with CMS

[kishore03109]

hmm correct me if I am wrong, but set setTimeout does not block the main thread right?

[harishv7]

@kishore03109 hmm, you can do a simple test on console -

async function test() { if(true) { await new Promise((resolve) => setTimeout(resolve, 2000)) } console.log("DONE") }

"DONE" wont be printed until the promise is done. you might want to double check the actual behaviour on this

[kishore03109]

Hmm wait isnt that the point? I want to only resolve this after 90 seconds to give Amplify enough time to generate the DNS results

eg for the code

async function test() {
  await new Promise((resolve) => setTimeout(resolve, 10000))
  console.log("PROMISE DONE")
}
console.log("STARTING")
test()
console.log("ENDED")

in the console log, we would see:
STARTING
ENDED
PROMISE DONE

thus the other functions in the thread will not be blocked?

[seaerchin]

think the confusion stems from the await - if you await in an async function:

const x = await work()
doOtherWork()
const y = x

the main thread is free to continue on until the result of the await-ed promise is used (ie, const y = x). however, because we await here, i think we'll pause execution for 90s

[harishv7]

@kishore03109 if you added await on test:

console.log("STARTING")
await test()
console.log("ENDED")

it will wait for promise to be done first

[kishore03109]

@harishv7, for ease of mind, I have made the main caller (this.handleSiteLaunchResults) function non-blocking (ie, removing the await), since we technically can run this async after returning the 200 response. However, this means that any error thrown by this will not be caught by handleSiteLaunchFormRequest. Therefore, I am wrapping a try catch around and logging the error to any unintended effects of some unexpected error. Ideally, during an error, this would return something to the user, but I think this can be revisited when we are integrating the features with the frontend!

[seaerchin]

i think this is non-blocking - top-most caller is handleSiteLaunchFormRequest in formsgSiteLaunch and it calls this.handleSiteLaunchResults(formResponses, submissionId) without await (which calls this method later on)

[harishv7]

I am concerned that future devs working around site launch feature might be confused why they are not able to create domain associations during development. Not too sure if there is a way to highlight clearly in code that the calls are mocks apart from the proposed solution of already naming them mockCreateDomainAssociationOutput and mockCreateDomainAssociationOutput in LaunchesClient. Open to feedback on this!

@kishore03109 maybe do you want to have an actual dev config like "MOCK_AMPLIFY_CALLS = true or false" which gives devs more "conscious" control over this? If you feel this could extend beyond in future as the launch service develops, you might want to have a great detail of specificity apart from isTestEnv().

[kishore03109]

@harishv7

@kishore03109 maybe do you want to have an actual dev config like "MOCK_AMPLIFY_CALLS = true or false" which gives devs more "conscious" control over this? If you feel this could extend beyond in future as the launch service develops, you might want to have a great detail of specificity apart from isTestEnv().

Hey thanks for the suggestion, think this is a cleaner solution, updated to use an env var. Regarding greater detail of specificity, I think we can revisit this as the service develops, I think this is ok for now!

[harishv7]

@harishv7

@kishore03109 maybe do you want to have an actual dev config like "MOCK_AMPLIFY_CALLS = true or false" which gives devs more "conscious" control over this? If you feel this could extend beyond in future as the launch service develops, you might want to have a great detail of specificity apart from isTestEnv().

Hey thanks for the suggestion, think this is a cleaner solution, updated to use an env var. Regarding greater detail of specificity, I think we can revisit this as the service develops, I think this is ok for now!

Sounds good, actually MOCK_AMPLIFY_CREATE_DOMAIN_CALLS is very specific already, as it specifies exactly that only "Create Domain" calls are mocked.

If there are a lot of mocking calls required, then a more generic env like MOCK_ALL_AMPLIFY_CALLS may be required. You can revise this in future if required

[seaerchin]

generally when we mock, we want to preserve the same function contract to ensure that calling code behaves as intended. moreover, because of how we use DI, we are able to substitute out the inner client (amplifyClient) for a mocked one, which ensures that our calling code is never aware of the fact that it's calling a fake client.

in here, the API contract is changed, which might lead to callers not upholding the contract and test logic is intermingled w/ actual biz logic, which seems a little messy.

wdyt about creating a MOCK_AMPLIFY_CLIENT that returns const data and passing in the appropriate client based on teh result of isTest?

[seaerchin]

think the confusion stems from the await - if you await in an async function:

const x = await work()
doOtherWork()
const y = x

the main thread is free to continue on until the result of the await-ed promise is used (ie, const y = x). however, because we await here, i think we'll pause execution for 90s

[kishore03109]

@seaerchin

generally when we mock, we want to preserve the same function contract to ensure that calling code behaves as intended. moreover, because of how we use DI, we are able to substitute out the inner client (amplifyClient) for a mocked one, which ensures that our calling code is never aware of the fact that it's calling a fake client.
in here, the API contract is changed, which might lead to callers not upholding the contract and test logic is intermingled w/ actual biz logic, which seems a little messy.
wdyt about creating a MOCK_AMPLIFY_CLIENT that returns const data and passing in the appropriate client based on teh result of isTest?

Yup, you are right to say that this is not ideal where test logic is intermingled with actual biz logic, and seems messy.

We primarily have two main calls to Amplify here:

1. CreateDomainAssociation
2. GetDomainAssociation

Originally, when actually calling Amplify, the getDomainAssociation only takes in the arguments domainName and appId, since the state of the application (specifically the SubDomainSettings[]) are stored in Amplify itself. To fully mock this behaviour, one must store this state, either in the database (which is def an over-engineered solution) or as a static variable in MockAmplifyClient. (Please let me know if there is another way that I missed :/ )

Say we store this state in some static variable AmplifyDomains in a new class MockAmplifyClient . To fully mock the GetDomainAssociation during development, one must continue to manipulate AmplifyDomains to fit the expected mocking values. I am foreseeing that this would make writing tests in the future harder, because it might not be immediately obvious to the developer that some variable in MockAmplifyClient needs to be modified in order for an accurate representation of what the Amplify Client would have returned if this was not mocked. This leads to an implicit dependency here, since the precondition for receiving an accurate mock from GetDomainAssociation is to set the static variable AmplifyDomains properly.

To be clear, changing the API contract here is a concession to give more power to the consumer to give a less ambiguous, clearer response of what the Amplify client will return if the values are not mocked. Does this reasoning make sense?

[seaerchin]

@seaerchin

generally when we mock, we want to preserve the same function contract to ensure that calling code behaves as intended. moreover, because of how we use DI, we are able to substitute out the inner client (amplifyClient) for a mocked one, which ensures that our calling code is never aware of the fact that it's calling a fake client.
in here, the API contract is changed, which might lead to callers not upholding the contract and test logic is intermingled w/ actual biz logic, which seems a little messy.
wdyt about creating a MOCK_AMPLIFY_CLIENT that returns const data and passing in the appropriate client based on teh result of isTest?

Yup, you are right to say that this is not ideal where test logic is intermingled with actual biz logic, and seems messy.

We primarily have two main calls to Amplify here:

1. CreateDomainAssociation
2. GetDomainAssociation

Originally, when actually calling Amplify, the getDomainAssociation only takes in the arguments domainName and appId, since the state of the application (specifically the SubDomainSettings[]) are stored in Amplify itself. To fully mock this behaviour, one must store this state, either in the database (which is def an over-engineered solution) or as a static variable in MockAmplifyClient. (Please let me know if there is another way that I missed :/ )

Say we store this state in some static variable AmplifyDomains in a new class MockAmplifyClient . To fully mock the GetDomainAssociation during development, one must continue to manipulate AmplifyDomains to fit the expected mocking values. I am foreseeing that this would make writing tests in the future harder, because it might not be immediately obvious to the developer that some variable in MockAmplifyClient needs to be modified in order for an accurate representation of what the Amplify Client would have returned if this was not mocked. This leads to an implicit dependency here, since the precondition for receiving an accurate mock from GetDomainAssociation is to set the static variable AmplifyDomains properly.

To be clear, changing the API contract here is a concession to give more power to the consumer to give a less ambiguous, clearer response of what the Amplify client will return if the values are not mocked. Does this reasoning make sense?

hmm i don't understand what you're trying to say here - this only applies if we ever hit the amplify endpoint, which we're not doing (by design). so why can't we just give a constant result of the expected shape back to the caller? we don't have to store anything/compute anything since the code in localdev won't ever hit amplify anyway, no?

[kishore03109]

@seaerchin

hmm i don't understand what you're trying to say here - this only applies if we ever hit the amplify endpoint, which we're not doing (by design). so why can't we just give a constant result of the expected shape back to the caller? we don't have to store anything/compute anything since the code in localdev won't ever hit amplify anyway, no?

the issue stems from the fact that the result of GetDomainAssociationCall semantically differs depending on arguments given to CreateDomainAssociation to trigger the domain association process.

super high level code to demonstrate this:

when actually calling Amplify:

createDomainAssociation({appId: blah, domainName: 'test.gov.sg', subDomainSettings: 'www'})
// some code to wait 90 secs
const res = getDomainAssociation({appId: blah, domainName:'test.gov.sg'})
if (res.subDomains) {
  // lets call this node in the CFG as node A 
  handleSubdomains(res) 
}  else {
  // lets call this node in the CFG as node B 
  handleDomainsWithoutSubdomains(res)
}

When mocking getDomainAssociation in local dev, if we returned an object of
const res = {
subDomains: "www"
...otherProps
}, node A will never be run.

When mocking getDomainAssociation in local dev, if we returned an object of
const res = {
...otherProps
}, node B will never be run.

To give more flexibility to the caller, the suggested solution proposes adding the prop subDomainSettings to fine-tune the mocked result returned!
Concretely I can call
getDomainAssociation({appId: blah, domainName: 'test.gov.sg', subDomainSettings: 'www'}) or
getDomainAssociation({appId: blah, domainName: 'test.gov.sg'}) depending on whether or not I wish to receive a mocked Response from amplify with subdomains or no subdomains.

[kishore03109]

@seaerchin as discussed offline, have modified to retain the original API contract. The state is now stored in LaunchesClient, who is the only one who is aware of any mocked values.

[seaerchin]

wrong method signature for InfraService.getDomainAssociationRecord call; minor comments otherwise

[seaerchin]

i think this is non-blocking - top-most caller is handleSiteLaunchFormRequest in formsgSiteLaunch and it calls this.handleSiteLaunchResults(formResponses, submissionId) without await (which calls this method later on)

[seaerchin]

lgtm