Use AJV to validate User JSON when getting a User by ID
#1464
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is necessary to get AJV to work so it can "tell the difference" between required and optional fields in a type definition.
This also pulls `UserRole` out into its own file. The new `validateUser` function can be used to determine whether any JavaScript value is in fact an instance of the type `User`. I had to make `_id` optional, since there are numerous places where we don't (yet) have an ID. As an example, when adding a user, we don't have an ID because that will come from the server when we actually add it.
The key part of this is the change to `getUserById`, where we now use the `validateUser()` function to check that the `user` value we got in the HTTP response from the server really is an instance of `User`. If it's not, we throw an error. The other changes to this file are introductions of `!` to say that code should fail is a value is `null`. For example: ```typescript return this.httpClient!.get<User[]>(... ``` the `!` says that if `this.httpClient` is `null` this should fail straightaway. This was only necessary because I had to say that the injected `httpClient` in the constructor could have the value `null`. I'm not happy about adding `null` as an option there, but I can figure out how to get the injection to work so that the mocked version of the service doesn't have to provide an `HttpClient`. I _think_ the solution to that is to get rid of the `MockUserService` and instead just stub what we need where we need it. Then we could get rid of `HttpClient | null`, and we wouldn't need all those `!`s.
There was a problem hiding this comment.
Copilot reviewed 3 out of 6 changed files in this pull request and generated no suggestions.
Files not reviewed (3)
- client/package-lock.json: Language not supported
- client/package.json: Language not supported
- client/tsconfig.json: Language not supported
Comments skipped due to low confidence (1)
client/src/app/users/user.service.ts:88
- The error message should provide more context about why the validation failed. Consider including details from the AJV validation errors.
throw new Error('User supplied by the server failed to validate: ' + user);
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces the AJV library, which we can use to validate JSON objects, i.e., confirm that they have all the right fields. At the moment, for example, the server could send any kind of weird stuff back when we request a
User, and the Angular code never really checks that or generates a meaningful error.This PR should be considered the start of a process, only checking one specific case, namely checking that the object
UserService#getUserByIdreceives from the HTTP request is actually an instance of theUsertype. If we merge this in, we would then want to do create issues for additional validation.The "important" parts of this PR are:
ajv"strictNullChecks": trueintsconfig.jsonajvto work; without it AJV can't tell properly distinguish between required and optional fields (I think).user.ts_idfield to optional since there are number places where we have aUserwithout an ID. Themain one is when someone enters a new
Userin a form, that won't (yet) have an ID. It's also an issue insome of the tests, where we don't provide a ID.
user-role.tsUserRoleout into its own file while we were here