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: "kishore@open.gov.sg",
    repoName: "kishore-test",
    primaryDomain: "kishorewithwww.isomer.gov.sg",
    redirectionDomain: "www.kishorewithwww.isomer.gov.sg",
    agencyEmail: "alexander@open.gov.sg",
  },
  {
    submissionId: "",
    requesterEmail: "kishore@open.gov.sg",
    repoName: "test-vincent",
    primaryDomain: "kishorewithoutwww.isomer.gov.sg",
    redirectionDomain: "",
    agencyEmail: "alexander@open.gov.sg",
  },
]

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]

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

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