feat: validate nRF Cloud API response #305

pudkrong · 2023-10-11T13:20:23Z

PR Type:

Enhancement

PR Description:

This PR introduces validation for the nRF Cloud API responses. It uses the AJV (Another JSON Schema Validator) library and the TypeBox library to define and validate the schema of the responses. In case of validation errors, a ValidationError is thrown. The PR affects multiple files, mainly focusing on implementing the validation in the fetch functions and updating the function calls accordingly.

PR Main Files Walkthrough:

files:

lambda/loggingFetch.ts: Added a ValidationError class and a validate function to validate the schema of the responses. Also, introduced a new function validatedLoggingFetch which validates the response before returning it.
lambda/resolveSingleCellGeoLocation.ts: Replaced loggingFetch with validatedLoggingFetch and updated the function calls to handle the validation errors.
nrfcloud/createAccountDevice.ts: Replaced the fetch call with a call to validatedFetch and handled the validation errors.
nrfcloud/devices.ts: Replaced the fetch call with a call to validatedFetch and handled the validation errors.
nrfcloud/getDeviceShadowFromnRFCloud.ts: Replaced the fetch call with a call to validatedFetch and handled the validation errors.
nrfcloud/validatedFetch.ts: Introduced a new function validatedFetch which validates the response before returning it.

codiumai-pr-agent-free · 2023-10-11T14:22:55Z

PR Analysis

🎯 Main theme: Implementing validation for nRF Cloud API responses
📝 PR summary: This PR introduces validation for the nRF Cloud API responses using the AJV (Another JSON Schema Validator) library and the TypeBox library. The validation is implemented in the fetch functions and the function calls are updated accordingly to handle validation errors.
📌 Type of PR: Enhancement
🧪 Relevant tests added: No
⏱️ Estimated effort to review [1-5]: 4, because the PR introduces significant changes to the codebase, including the addition of new functions and modifications to existing ones. The changes are spread across multiple files, which increases the complexity of the review.
🔒 Security concerns: No security concerns found

PR Feedback

💡 General suggestions: The PR is well-structured and the changes are logically grouped. The use of AJV and TypeBox for validation is a good approach. However, it would be beneficial to add tests to ensure the validation works as expected and does not introduce any regressions.
🤖 Code feedback:
- relevant file: lambda/loggingFetch.ts
  suggestion: Consider handling the case where 'errors' is not in 'maybeData' in the 'validate' function. This will ensure that the function behaves as expected even when the validation does not return any errors. [important]
  relevant line: '+ if ('errors' in maybeData) {'
- relevant file: lambda/resolveSingleCellGeoLocation.ts
  suggestion: It would be better to handle the case where 'error' is not in 'maybeResult' in the 'serviceToken' function. This will ensure that the function behaves as expected even when the validation does not return any errors. [important]
  relevant line: '+ if ('error' in maybeResult) {'
- relevant file: nrfcloud/createAccountDevice.ts
  suggestion: Consider handling the case where 'error' is not in 'maybeResult' in the 'createAccountDevice' function. This will ensure that the function behaves as expected even when the validation does not return any errors. [important]
  relevant line: '+ if ('error' in maybeResult) {'
- relevant file: nrfcloud/devices.ts
  suggestion: It would be better to handle the case where 'error' is not in 'maybeResult' in the 'devices' function. This will ensure that the function behaves as expected even when the validation does not return any errors. [important]
  relevant line: '+ if ('error' in maybeResult) {'

How to use

To invoke the PR-Agent, add a comment using one of the following commands:
/review [-i]: Request a review of your Pull Request. For an incremental review, which only considers changes since the last review, include the '-i' option.
/describe: Modify the PR title and description based on the contents of the PR.
/improve [--extended]: Suggest improvements to the code in the PR. Extended mode employs several calls, and provides a more thorough feedback.
/ask <QUESTION>: Pose a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.

To edit any configuration parameter from configuration.toml, add --config_path=new_value
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, use the /config command.

coderbyheart · 2023-10-11T14:27:29Z

lambda/resolveSingleCellGeoLocation.ts

 const { track, metrics } = metricsForComponent('singleCellGeo')

-const trackFetch = loggingFetch({ track, log })
+const trackFetch = validatedLoggingFetch({ track, log })


Suggested change

const trackFetch = validatedLoggingFetch({ track, log })

const trackFetch = validatedFetch(loggingFetch({ track, log }))

Since loggingFetch returns a fetch compatible function, validatedLoggingFetch should not care about the fact that it deals with a fetch that is augmented with logging capabilites. Instead make it accept a fetch instance as parameter. (Unix command design principle)

I will use the existing function, validatedFetch, because

It accepts fetch instance

However, I need to modify the function to accept 3rd parameter because the existing function will use GET method and Content-Type as application/json as its defaults.

lambda/loggingFetch.ts

lambda/resolveSingleCellGeoLocation.ts

coderbyheart · 2023-10-11T14:34:07Z

lambda/resolveSingleCellGeoLocation.ts

+
+		if ('error' in maybeResult) {
+			log.error('request failed', { error: maybeResult.error })
+			throw new Error(`Acquiring service token failed: ${maybeResult.error}`)


comment: this swallows the validation errors (because the error is stringified). But OK in this case because error is logged and function is only used in lambda.

coderbyheart · 2023-10-11T14:36:09Z

nrfcloud/createAccountDevice.ts

+
+	if ('error' in maybeResult) {
+		console.error(`Failed to create account device:`, maybeResult.error)
+		throw new Error(`Failed to create account device`)


problem: this swallows the error details, change API and return error

I will re-throw the error from validatedFetch. The error can be either Error object or ValidationError.

nrfcloud/devices.ts

nrfcloud/getDeviceShadowFromnRFCloud.ts

nrfcloud/validatedFetch.ts

coderbyheart · 2023-10-11T14:39:37Z

nrfcloud/validatedFetch.ts

 	async <Schema extends TObject>(
 		{ resource }: { resource: string },
 		schema: Schema,
+		init?: RequestInit,


Unrelated change.

This change is necessary due to the default behavior uses GET method and Content-Type as application/json

Changed in the refactor.

pudkrong · 2023-10-11T17:24:31Z

lambda/fetchDeviceShadow.ts

 					Math.round(Date.now() / 1000) -
-						getShadowUpdateTime(deviceShadow.state.metadata),
+						getShadowUpdateTime(
+							deviceShadow.state.metadata as Record<string, any>,


I need to cast deviceShadow.state.metadata to Record<string, any> because getShadowUpdateTime requires

Yeah, recursive typing is not really possible with TypeBox (and JSON schema)

pudkrong · 2023-10-11T17:25:38Z

lambda/loggingFetch.ts

 	({ track, log }: { track: AddMetricsFn; log: Logger }) =>
-	async (url: URL, init?: RequestInit): ReturnType<typeof fetch> => {
+	async (
+		url: URL | RequestInfo,


This is required to allow loggingFetch to be able to pass as fetchImplementation

I've addressed this in a refactor.

This updates the validateFetch API and integrates the fetch configuration in the first parameter. This way there are not two parameters (that are separated by the schema parameter) that control what fetch sends. It also adds convenience functions to provide payloads and sends the correct content-type header only if needed.

coderbyheart · 2023-10-12T08:38:58Z

lambda/validatedFetch.ts

File is empty.

coderbyheart · 2023-10-12T09:15:53Z

lambda/resolveSingleCellGeoLocation.ts

-		const res = await trackFetch(
-			new URL(`${slashless(apiEndpoint)}/v1/location/ground-fix`),
+
+		const vf = validatedFetch({ endpoint: apiEndpoint, apiKey }, trackFetch)


locationServiceToken can be passed here.

coderbyheart · 2023-10-12T09:45:40Z

lambda/fetchDeviceShadow.ts

 					Math.round(Date.now() / 1000) -
-						getShadowUpdateTime(deviceShadow.state.metadata),
+						getShadowUpdateTime(
+							deviceShadow.state.metadata as Record<string, any>,


Yeah, recursive typing is not really possible with TypeBox (and JSON schema)

coderbyheart · 2023-10-12T09:46:36Z

nrfcloud/getDeviceShadowFromnRFCloud.ts

+/**
+ * @link https://api.nrfcloud.com/v1/#tag/All-Devices/operation/ListDevices
+ */
+const DeviceShadow = Type.Object({


Should have been integrated in DeviceShadow.ts

coderbyheart · 2023-10-12T09:47:00Z

lambda/loggingFetch.ts

 	({ track, log }: { track: AddMetricsFn; log: Logger }) =>
-	async (url: URL, init?: RequestInit): ReturnType<typeof fetch> => {
+	async (
+		url: URL | RequestInfo,


I've addressed this in a refactor.

coderbyheart · 2023-10-12T09:49:04Z

nrfcloud/validatedFetch.spec.ts

+		const res = await vf({ resource: 'foo' }, schema, {
+			method: 'POST',
+			headers: {
+				Authorization: 'Bearer another-key',
+				'Content-Type': 'application/octet-stream',
+			},
+		})


If refactored this, because this mixes two parameters to configure the fetch behaviour. Those two (the first and the third) are also split around the schema.

coderbyheart · 2023-10-12T09:49:22Z

nrfcloud/validatedFetch.ts

 	async <Schema extends TObject>(
 		{ resource }: { resource: string },
 		schema: Schema,
+		init?: RequestInit,


Changed in the refactor.

See #305

pudkrong linked an issue Oct 11, 2023 that may be closed by this pull request

Implement nRF Cloud API response validation #291

Closed

5 tasks

feat: validate nRF Cloud API response

6856831

pudkrong force-pushed the 291-implement-nrf-cloud-api-response-validation branch from b9fffe7 to 6856831 Compare October 11, 2023 14:05

pudkrong temporarily deployed to ci October 11, 2023 14:05 — with GitHub Actions Inactive

coderbyheart mentioned this pull request Oct 11, 2023

Help wanted bifravst/help-wanted#2

Open

pudkrong marked this pull request as ready for review October 11, 2023 14:20

pudkrong requested a review from coderbyheart as a code owner October 11, 2023 14:20

codiumai-pr-agent-free bot added the enhancement New feature or request label Oct 11, 2023

coderbyheart requested changes Oct 11, 2023

View reviewed changes

feat: reuse validatedFetch

57f6f00

pudkrong temporarily deployed to ci October 11, 2023 17:20 — with GitHub Actions Inactive

pudkrong commented Oct 11, 2023

View reviewed changes

pudkrong requested a review from coderbyheart October 11, 2023 17:37

pudkrong temporarily deployed to ci October 11, 2023 20:07 — with GitHub Actions Inactive

coderbyheart added 3 commits October 12, 2023 10:39

fix: remove empty file

795db45

refactor: improve DeviceShadow type and merge with existing type

ef5e92c

coderbyheart approved these changes Oct 12, 2023

View reviewed changes

coderbyheart temporarily deployed to ci October 12, 2023 09:50 — with GitHub Actions Inactive

coderbyheart merged commit 6781fbb into saga Oct 12, 2023

coderbyheart deleted the 291-implement-nrf-cloud-api-response-validation branch October 12, 2023 10:44

coderbyheart added a commit that referenced this pull request Oct 12, 2023

fix(nrfcloud): pageNextToken is not required

47e11e4

See #305

	const trackFetch = validatedLoggingFetch({ track, log })
	const trackFetch = validatedFetch(loggingFetch({ track, log }))

feat: validate nRF Cloud API response #305

feat: validate nRF Cloud API response #305

Uh oh!

Conversation

pudkrong commented Oct 11, 2023 • edited by codiumai-pr-agent-free bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Type:

PR Description:

PR Main Files Walkthrough:

Uh oh!

codiumai-pr-agent-free bot commented Oct 11, 2023

PR Analysis

PR Feedback

How to use

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pudkrong commented Oct 11, 2023 •

edited by codiumai-pr-agent-free bot

Loading