Backend Challenge Solution - Manan Habib #23

manan-habib · 2022-08-11T02:37:14Z

No description provided.

…olution

Added timeout in e2e tests to fix timeout issue

marcelino-sf

Thank you @mananhabib31 for taking your time and working on this code challenge. I left a few questions to understand your thinking process and to get your thoughts.

marcelino-sf · 2022-08-12T14:24:05Z

coverage/clover.xml

@@ -0,0 +1,101 @@
+<?xml version="1.0" encoding="UTF-8"?>


What was the reason you decided to push the coverage folder?

There is no major reason. I just had one thought to let reviewer quickly go over the html report(./coverage/lcov-report/index.html) that was generated before my push.

marcelino-sf · 2022-08-12T14:25:17Z

main.tf

@@ -0,0 +1,68 @@
+variable "graphql_api_name" {


what happens if we grow the project with dozen of variables, and a lot more infrastructure and modules, would it be nice to have them defined in main.tf?

Nope it wouldn't, if the scope of the project gets bigger and we need more modules and variables to define then we should be adding separate files for each module and variables. If variables grow more then variable file can be broken down on the basis of cohesion of variables. I didn't create a separate one as in the scope of this project I didn't think that we need to.

marcelino-sf · 2022-08-12T14:25:57Z

package.json

+    "reflect-metadata": "^0.1.13",
+    "ts-node": "^10.9.1",
+    "typedi": "^0.10.0",
+    "typescript": "^4.7.4"


I can see some developer dependencies defined in this section, what was the reason you decided to add them as production dependencies?

Yeah, you are right. That's a mistake from my side. I should have used -D or --save-dev while installing these. My bad!

marcelino-sf · 2022-08-12T14:26:46Z

src/configs/config.ts

@@ -0,0 +1,27 @@
+import dotenv from 'dotenv';


what was the reason you wanted to have dotenv in production w/ lambdas?

This particular thing I have inherited from my .net core experience. In .net core, developers prefer to have appsettings file and one can even override the env variable by using appsettings json file. It's just more continent to use settings file because .net framework itself supports better maintainability of by allowing them to be broken down on cohesion basis.We are using the same thing in our identity server app as well. But seeing your comment on this thing making me realize things are different in nodejs. In simple words, it's just due to past experience.

marcelino-sf · 2022-08-12T14:28:46Z

aws.env

+NODE_ENV="aws"
+
+# Google Geocode API
+GOOGLE_MAPS_KEY='AIzaSyBj1aLo0X1uH7CKoi3bXPFPdX6eeCra4e8'


Why did you want to push this API key to the repo? This is a sensitive value

I encourage you to remove it as quickly as possible from your google console before someone malicious start using it causing undexpected charges on your credit card

Well you are right that its a sensitive info and the only motivation behind adding this was just to let reviewer instantly test the functionality(locally) without configuring its own key. To be on safe side, I already have a cap on budget of 5$ monthly to avoid any surprise situation.

marcelino-sf · 2022-08-12T14:30:26Z

aws.env

+GOOGLE_MAPS_KEY='AIzaSyBj1aLo0X1uH7CKoi3bXPFPdX6eeCra4e8'
+
+# Debug
+LOG_LEVEL='error'


Help me understand why you added just error logs to the AWS lambda in production. What is your opinion about INFO, WARN, DEBUG, and TRACE logs in production?

Well there was no specific reason to set level to 'error' in aws env file. I was just testing all the levels with cloudwatch and ended up on error. To answer your question that what should we be doing in production then under normal circumstances production log level should be 'Info'. As far as the different log levels are concerned:

Info: These logs gives useful information about the system in general. This is sort of information which one don't really care about under normal circumstances but very helpful in crisis situation.
WARN: Gives you info about anything that has a potential to cause error OR fatal exception. Usually we have mechanisms for auto-recovery(or system gets recovered on its own sometimes) with problems logged on this level but this keeps us alerted that what can go wrong in future.
Debug/Trace: I would like to address debug and trace in same part. There is always a debate between level of these log levels in hierarchy but in my experience, debug < trace. This is the level which we only turn on in the situation of diagnoses. If we consider debug < trace side, then debug logs fine grained information(but less granular than Trace) about operation being performed while trace level gives us the most fine grained info(even step by step sometimes) about the operation. We only enable it to diagnose the things in production because it has a performance impact on system.

Note: I assumed that we are discussing log TRACE level and not distributed tracing in terms of microservices.

marcelino-sf · 2022-08-12T14:31:04Z

package.linux.json

@@ -0,0 +1,38 @@
+{


why do we need to have a separate package.json file for linux?

Because I developed this project on a windows machine and some commands in package file are windows based e.g. copy file command. So after completing my work I spinned up a linux machine and created another working package file for reviewer(assuming that reviewer would have linux based system).

marcelino-sf · 2022-08-12T14:32:14Z

src/services/locationService.ts

+            }
+        } catch (error) {
+            /* istanbul ignore next */ 
+            this.logger.error(`locationService.getCoordinates ended up in exception. Error: ${error}`);


is this error being exposed to the client somehow? I meean, when there's an error in this service, do the client know what happened or they just receive an arbitrary message?

It wouldn't end up to send arbitrary message but a null object in response which you are right about that it should be sending an error. I made a mistake there and overlooked this place to return error object.

marcelino-sf · 2022-08-12T14:33:21Z

tests/unit/schema/customScalars.test.ts

+
+      test('Should return error for empty string value', async () => {
+        try {
+          var result = NonEmptyString.serialize('');


I can see by the title that the test is expecting an error, but if this line doesn't throw anything it will pass the test anyway

Yeah, you are right I skipped that case where if someone stop throwing the error from subject method then it will pass anyway. I should have used following instead of current expect statement:
expect(NonEmptyString.serialize('')).toThrowError({message: Value can't be empty, name: 'VALUE_NOT_EMPTY'});

marcelino-sf · 2022-08-12T14:37:35Z

src/resolvers/coordinates/query.ts

@@ -0,0 +1,5 @@
+export class CoordinatesQuery {
+    public async getCoordinates(parent: any, args: any, {dataSources}: any, info: any){


why did you want to use any instead of actual types? Wouldn't using types better since we're using TypeScript?

The only reason I didn't specify types there is that this resolver just wire down the request to location service and do nothing else significant but yeah in context of typescript usage specifying types would make more sense.

Removed map api key

manan-habib added 24 commits August 10, 2022 05:46

Initial commit

6c7f6ad

Updated readme with architecture, structure and stack details

13f6542

Added project architecture image

496897b

Deleting file

511a919

Added project architecture image

9b8481d

Linked architecture image in readme

457460b

Added details about environment variables, building and shipping of s…

41923df

…olution

Removed the duplication

e708dcc

Added api reference

76a8325

Added sample request

1150c74

Added implementation details

945e3e1

Added link

30f550f

Fixed table of contents

4ec2659

Linked table of contents

f66a9f5

Removed duplication

b48685a

Changed aws key and id

e184ec1

Added timeout in e2e tests to fix timeout issue

Merge branch 'main' of https://github.com/mananhabib31/cloud-backend-…

10e149b

…test into main

Updated readme with prereqs

220be47

removed aws keys

badd2e9

Added description in schema

678ba3e

Merge branch 'main' of https://github.com/mananhabib31/cloud-backend-…

d160587

…test into main

Added demo section

4aaf9af

Removed some duplication

e7bd394

Added details about terraform variables

2364cc8

marcelino-sf reviewed Aug 12, 2022

View reviewed changes

manan-habib added 3 commits August 15, 2022 05:59

Removed map api key

2bd2ccf

removed map api key

e3e9052

Update development.env

c5b32e3

Removed map api key

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backend Challenge Solution - Manan Habib #23

Backend Challenge Solution - Manan Habib #23

manan-habib commented Aug 11, 2022

marcelino-sf left a comment

marcelino-sf Aug 12, 2022

manan-habib Aug 14, 2022 •

edited

Loading

marcelino-sf Aug 12, 2022

manan-habib Aug 14, 2022

marcelino-sf Aug 12, 2022

manan-habib Aug 14, 2022

marcelino-sf Aug 12, 2022

manan-habib Aug 14, 2022 •

edited

Loading

marcelino-sf Aug 12, 2022

manan-habib Aug 14, 2022

marcelino-sf Aug 12, 2022

manan-habib Aug 14, 2022 •

edited

Loading

marcelino-sf Aug 12, 2022

manan-habib Aug 14, 2022

marcelino-sf Aug 12, 2022

manan-habib Aug 14, 2022

marcelino-sf Aug 12, 2022

manan-habib Aug 14, 2022 •

edited

Loading

marcelino-sf Aug 12, 2022

manan-habib Aug 14, 2022

		@@ -0,0 +1,5 @@
		export class CoordinatesQuery {
		public async getCoordinates(parent: any, args: any, {dataSources}: any, info: any){

Backend Challenge Solution - Manan Habib #23

Are you sure you want to change the base?

Backend Challenge Solution - Manan Habib #23

Conversation

manan-habib commented Aug 11, 2022

marcelino-sf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manan-habib Aug 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manan-habib Aug 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manan-habib Aug 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manan-habib Aug 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manan-habib Aug 14, 2022 •

edited

Loading

manan-habib Aug 14, 2022 •

edited

Loading

manan-habib Aug 14, 2022 •

edited

Loading

manan-habib Aug 14, 2022 •

edited

Loading