Verbose but working partial implementation of creating change requests #413
Conversation
There was a problem hiding this comment.
Just leaving suggestions to clean up code. Logically all of this code makes sense. I trust that you've verified the code works and tested it. I do not know why each field needs to be validated, so I'll leave that to Alexander.
To your note on the route-matching system. I can think on this for a bit, but it might be wise to start re-architecting the backend before this application gets too big. Maybe working on that in parallel with new features/bug fixes this semester.
TLDR;
- Yes I agree, validation library or manually abstract validation functions
- I like middy.js the best as well. 2nd choice is the simple json-schem-to-ts
- Logging question: What are we logging and why?
- If we want to redesign backend, let's consider how we'd plan this out. Let's discuss this on a call or in a back and forth chat.
- LGTM
| @@ -110,12 +111,102 @@ const getChangeRequestByID: ApiRouteFunction = async (params: { id: string }) => | |||
| return buildSuccessResponse(changeRequestTransformer(requestedCR)); | |||
| }; | |||
|
|
|||
| const createNewChangeRequest: ApiRouteFunction = async (_params, event) => { | |||
| if (event.body === undefined) { | |||
There was a problem hiding this comment.
A majority of these if statements serve to validate the data that we want. I'm going to provide two solutions below that will help make this code more readable.
My first suggestion is adding a validation library as you mentioned. Here are some in addition to the ones you listed from the article below:
- https://github.com/ajv-validator/ajv
- https://github.com/hapijs/joi
- https://github.com/imbrn/v8n
- https://github.com/ansman/validate.js
I agree with your choice that Middy.js looks like the best pick.
For people who are not James and not super familiar with "validation"
Essentially these libraries let us define schemas for the data we want. Within these schemas we can say some fields are non-null or this must be a boolean or this must be an integer between 1-10 or define enums.
Here's a quick blog post to skim: https://blog.logrocket.com/dynamic-type-validation-in-typescript/
There was a problem hiding this comment.
I'm strongly in favor of evaluating a validation library to reduce the work we need to do, but it still takes time to understand even just one validation library to consider and I'm very much a back-end novice here. I agree Middy.js seems like a great choice, but one hesitation is that it seems clear to me from their GitHub and from my test implementation (#414) that there isn't great TypeScript support. I don't currently think that's a deal-breaker though, but I haven't really evaluated any other packages.
| return buildClientFailureResponse('No data for CR creation.'); | ||
| } | ||
| const body = JSON.parse(event.body!); | ||
| if ( |
There was a problem hiding this comment.
If we are not interested in adding new libraries, then we might want to abstract each of these if statements into validation rules. Even if each function is only used once, it helps the main function be readable by distilling each if statement into a function with a descriptive name that says what it's doing.
We would turn:
if ( body.submitterId === undefined || body.wbsElementId === undefined || body.type === undefined ) { return buildClientFailureResponse('Missing common CR field(s).'); }
Into:
validateCommonCrFields(body)
My main reason for this suggestion is that future methods could either use the same validation rules or at the very least, someone reading this function for the first time can quickly see several validation function calls and understand that each of those functions is checking a few fields.
There was a problem hiding this comment.
The last time we opted to abstract stuff ourselves rather than go for a library I regretted it cuz it ended up being a big headache and wasting a lot of time given how good and feature-rich React-Query turned out to be. We ended up switching to the package as soon as we hit the more complicated use case. I don't wanna make the same mistake here, but yeah I'm on the same page with abstraction.
| return buildClientFailureResponse('ID field(s) must be integers.'); | ||
| } | ||
| if ( | ||
| body.type === CR_Type.ISSUE || |
There was a problem hiding this comment.
In my opinion, I think this if statement is what's most critical to understand in this code, but please correct me I'm mistaken. We want to know what type of Change Request it is and then act accordingly. If it's any of these three, we make our Prisma call and actually do stuff. If it's a STAGE_GATE or ACTIVATION, we return those respective responses without doing anything here.
That information is harder to see with all of the validation rules inside of these if statements i.e. lines 134 - 142
There was a problem hiding this comment.
^The above is me justifying the added cost of a validation library or even taking time manually abstracting functions
There was a problem hiding this comment.
100% agreed, it's messy and tough to read this implementation
| ) { | ||
| return buildClientFailureResponse('Missing standard CR field(s).'); | ||
| } | ||
| console.log(body.timelineImpact >= 0); |
There was a problem hiding this comment.
I can't speak for the rest of the code base, but I'm wondering about logging this "timeline impact." I'm wondering about logging the CR type that was detected when this function was called. Is there documentation somewhere on what we're logging in general and why?
There was a problem hiding this comment.
This log line is totally meaningless. I logged stuff to see what was going on / debug. I haven't even begun to think about actual purposeful logging.
|
I really love the conversation this has brought up! For anyone else coming to look at this PR, note that the Middy.js implementation over in PR #414 is also super important to review. Given how bleh this implementation is, I suspect most of the interesting conversation will go into #414. Definitely should revisit logging at some point later though. |
|
Replaced by successful #414 |
Oh lordy. Perhaps we should re-think the way we've got a route-matching system built on top of the lambdas. My lack of backend experience is really showing here. This does work for creating standard scope change requests, but it isn't a complete implementation.
The tools that would make validation so so so immensely easier also push us towards more "proper" setup of our lambdas.
middy.js is the main wholistic package I'm looking at potentially providing the most value for us to adopt, even if it would mean tossing out a bunch of work we've done (deja vu, see our late adoption of React-Query) and re-architecting our backend (probably actually a good thing, just takes work).
I've also looked at a few other packages, both more and less popular, but it's tough to say which might be appropriate. json-schem-to-ts seems interesting given our pure TS stack and potentially could be low negative impact, yup is immensely more popular but I'm honestly not sure how I feel about the function-based design, and zod is cool but hooks into a deeper ecosystem that I'm not sure we want to dive into.