Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Don't log 4xx as error #14649

Merged
merged 6 commits into from
May 22, 2024
Merged

chore: Don't log 4xx as error #14649

merged 6 commits into from
May 22, 2024

Conversation

AndesKrrrrrrrrrrr
Copy link
Member

@AndesKrrrrrrrrrrr AndesKrrrrrrrrrrr commented Apr 30, 2024

Why is this even a feature?

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Formatting passes locally with my changes
  • I have rebased against main before asking for a review

Summary by CodeRabbit

  • Configuration Updates
    • Improved error handling by removing the treat400ResponsesAsErrors option from various API configurations. This change standardizes how 400 responses are processed across multiple modules, enhancing consistency and reliability in error management.

Copy link
Contributor

coderabbitai bot commented Apr 30, 2024

Walkthrough

The overall change involves the removal of the treat400ResponsesAsErrors configuration option from various API configuration files across multiple modules. This update affects how 400 HTTP responses (client errors) are handled, ensuring that they are no longer automatically treated as errors within the API client logic. The change spans multiple libraries, including those related to occupational safety, aircraft registry, housing benefits, Icelandic government vacancies, health insurance, and middleware functionalities.

Changes

Files Change Summary
libs/clients/administration-of-occupational-safety-and-health/.../api.provider.ts Removed treat400ResponsesAsErrors from ApiConfig.
libs/clients/aircraft-registry/src/lib/apiConfig.ts Removed treat400ResponsesAsErrors from ApiConfig.
libs/clients/housing-benefit-calculator/src/lib/housing-benefit-calculator.provider.ts Removed treat400ResponsesAsErrors from ApiConfig.
libs/clients/icelandic-government-institution-vacancies/src/lib/apiConfig.ts Removed treat400ResponsesAsErrors from ApiConfig.
libs/clients/icelandic-health-insurance/health-insurance/.../clients-health-insurance-v2.module.ts Removed treat400ResponsesAsErrors from fetchApi configuration in HealthInsuranceV2Client class.
libs/clients/middlewares/README.md Removed treat400ResponsesAsErrors from createEnhancedFetch function configuration.
libs/clients/middlewares/src/lib/createEnhancedFetch.spec.ts Removed test case related to configuring the circuit to open for 400 errors.
libs/clients/middlewares/src/lib/createEnhancedFetch.ts Removed treat400ResponsesAsErrors from EnhancedFetchOptions interface and createEnhancedFetch function.
libs/clients/middlewares/src/lib/withCircuitBreaker.ts Removed treat400ResponsesAsErrors from CircuitBreakerOptions and updated errorFilter logic.
libs/clients/middlewares/src/lib/withErrorLog.ts Removed treat400ResponsesAsErrors from withErrorLog function and adjusted error handling logic.
libs/clients/ship-registry/src/lib/ship-registry.provider.ts Removed treat400ResponsesAsErrors from fetchApi configuration in ApiConfig.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@datadog-island-is
Copy link

datadog-island-is bot commented Apr 30, 2024

Datadog Report

All test runs 7f96dab 🔗

46 Total Test Services: 0 Failed, 45 Passed
🔻 Test Sessions change in coverage: 13 decreased, 9 increased, 178 no change

Test Services
This report shows up to 10 services
Service Name Failed Known Flaky New Flaky Passed Skipped Total Time Code Coverage Change Test Service View
air-discount-scheme-backend 0 0 0 81 0 37.1s 1 no change Link
api 0 0 0 4 0 4.89s 1 no change Link
api-domains-air-discount-scheme 0 0 0 6 0 34.6s 1 increased (+0.02%) Link
api-domains-assets 0 0 0 3 0 22s 1 no change Link
api-domains-auth-admin 0 0 0 18 0 21.29s 1 no change Link
api-domains-communications 0 0 0 5 0 56.38s 1 no change Link
api-domains-criminal-record 0 0 0 5 0 15.07s 1 decreased (-0.06%) Link
api-domains-driving-license 0 0 0 23 0 1m 1.14s 1 increased (+0.02%) Link
api-domains-education 0 0 0 8 0 37.73s 1 decreased (-0.03%) Link
api-domains-health-insurance 0 0 0 4 0 17.42s 1 decreased (-0.06%) Link

🔻 Code Coverage Decreases vs Default Branch (13)

This report shows up to 5 code coverage decreases.

  • clients-middlewares - jest 75.96% (-0.33%) - Details
  • services-user-notification - jest 48.74% (-0.1%) - Details
  • services-auth-ids-api - jest 51.18% (-0.1%) - Details
  • api-domains-criminal-record - jest 46.29% (-0.06%) - Details
  • api-domains-health-insurance - jest 29.78% (-0.06%) - Details

Copy link

codecov bot commented Apr 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 37.01%. Comparing base (03ef9bd) to head (8c35362).

Current head 8c35362 differs from pull request most recent head b655272

Please upload reports for the commit b655272 to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #14649      +/-   ##
==========================================
- Coverage   37.04%   37.01%   -0.04%     
==========================================
  Files        6331     6298      -33     
  Lines      129138   127752    -1386     
  Branches    36865    36534     -331     
==========================================
- Hits        47838    47286     -552     
+ Misses      81300    80466     -834     
Flag Coverage Δ
air-discount-scheme-backend 54.11% <33.33%> (+0.19%) ⬆️
api 3.44% <ø> (-0.05%) ⬇️
api-domains-air-discount-scheme 36.28% <0.00%> (+0.12%) ⬆️
api-domains-assets 26.71% <ø> (ø)
api-domains-auth-admin 57.99% <ø> (+0.07%) ⬆️
api-domains-communications 40.38% <0.00%> (-0.22%) ⬇️
api-domains-criminal-record 43.74% <83.33%> (-0.10%) ⬇️
api-domains-driving-license 44.70% <83.33%> (+0.13%) ⬆️
api-domains-education 33.83% <33.33%> (+0.02%) ⬆️
api-domains-health-insurance 27.34% <33.33%> (-0.01%) ⬇️
api-domains-mortgage-certificate 35.05% <33.33%> (+0.02%) ⬆️
api-domains-payment-schedule 40.64% <0.00%> (+0.14%) ⬆️
application-api-files 56.49% <ø> (-0.03%) ⬇️
application-system-api 42.08% <33.33%> (+0.13%) ⬆️
application-template-api-modules 24.66% <0.00%> (+0.27%) ⬆️
application-templates-car-recycling 4.23% <ø> (ø)
application-templates-criminal-record 21.57% <ø> (-0.11%) ⬇️
application-templates-driving-license 16.71% <ø> (+0.18%) ⬆️
application-templates-estate 11.73% <ø> (-0.02%) ⬇️
application-templates-example-payment 20.39% <ø> (-0.11%) ⬇️
application-templates-inheritance-report 3.89% <ø> (-0.06%) ⬇️
application-templates-mortgage-certificate 43.89% <ø> (-0.21%) ⬇️
application-ui-shell 21.71% <ø> (-0.02%) ⬇️
auth-api-lib 10.00% <ø> (+0.18%) ⬆️
clients-charge-fjs-v2 22.58% <ø> (ø)
clients-driving-license 40.48% <33.33%> (+0.11%) ⬆️
clients-driving-license-book 43.92% <83.33%> (-0.11%) ⬇️
clients-financial-statements-inao 48.81% <33.33%> (+0.10%) ⬆️
clients-license-client 1.84% <ø> (ø)
clients-middlewares 73.36% <100.00%> (-0.27%) ⬇️
clients-regulations 42.26% <83.33%> (-0.01%) ⬇️
clients-rsk-company-registry 29.46% <0.00%> (+0.22%) ⬆️
clients-syslumenn 49.76% <100.00%> (-0.12%) ⬇️
cms 0.44% <ø> (+<0.01%) ⬆️
cms-translations 39.47% <0.00%> (-0.24%) ⬇️
download-service 44.33% <33.33%> (-0.05%) ⬇️
judicial-system-api 17.60% <ø> (-0.01%) ⬇️
judicial-system-backend 55.75% <0.00%> (-0.36%) ⬇️
license-api 43.09% <33.33%> (-0.18%) ⬇️
services-auth-admin-api 50.65% <0.00%> (-0.57%) ⬇️
services-auth-delegation-api 62.49% <83.33%> (+0.52%) ⬆️
services-auth-ids-api 54.74% <33.33%> (+0.38%) ⬆️
services-auth-personal-representative 49.93% <33.33%> (+0.23%) ⬆️
services-auth-personal-representative-public 45.50% <0.00%> (+0.21%) ⬆️
services-auth-public-api 50.32% <33.33%> (+0.38%) ⬆️
services-endorsements-api 55.51% <33.33%> (+0.13%) ⬆️
services-university-gateway 48.45% <33.33%> (-0.05%) ⬇️
services-user-notification 47.64% <33.33%> (-0.16%) ⬇️
services-user-profile 62.81% <33.33%> (-0.04%) ⬇️
web 1.91% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...ance/src/lib/clients-health-insurance-v2.module.ts 100.00% <ø> (ø)
...clients/middlewares/src/lib/createEnhancedFetch.ts 100.00% <ø> (ø)
.../clients/middlewares/src/lib/withCircuitBreaker.ts 93.33% <100.00%> (-0.79%) ⬇️
libs/clients/middlewares/src/lib/withErrorLog.ts 100.00% <100.00%> (ø)

... and 282 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03ef9bd...b655272. Read the comment docs.

@saevarma saevarma removed their request for review May 14, 2024 10:45
Copy link
Member

@saevarma saevarma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀
Merge this?

@saevarma saevarma marked this pull request as ready for review May 16, 2024 12:39
@saevarma saevarma requested review from a team as code owners May 16, 2024 12:39
@saevarma saevarma added the deprecated:automerge (Disabled) Merge this PR as soon as all checks pass label May 16, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Out of diff range and nitpick comments (8)
libs/clients/middlewares/README.md (8)

Line range hint 15-15: Add a comma after "By default".

- By default nothing is cached.
+ By default, nothing is cached.

Line range hint 47-47: Add a comma after "Parsed JSON or string".

- Parsed JSON or string depending on the response content type.
+ Parsed JSON or string, depending on the response content type.

Line range hint 88-88: Add a comma after "In both modes".

- In both modes you specify these options:
+ In both modes, you specify these options:

Line range hint 273-273: Add a comma after "In this case".

- In this case it will immediately update the cache in the background to return fresher data in future requests.
+ In this case, it will immediately update the cache in the background to return fresher data in future requests.

Line range hint 278-278: Add a comma after "special consideration".

- need special consideration since those won't be stored by default.
+ need special consideration, since those won't be stored by default.

Line range hint 296-296: Use "e.g." instead of "eg".

- (eg from [@CurrentUser](../../auth-nest-tools/README.md#using-in-rest-controller))
+ (e.g. from [@CurrentUser](../../auth-nest-tools/README.md#using-in-rest-controller))

Line range hint 312-312: Add a comma before "and".

- then a warning is logged and the cache is disabled.
+ then a warning is logged, and the cache is disabled.

Line range hint 337-337: Use "API-specific" instead of "API specific".

- to add the API specific header keys.
+ to add the API-specific header keys.
Review Details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits Files that changed from the base of the PR and between df7f541 and 8c35362.
Files selected for processing (11)
  • libs/clients/administration-of-occupational-safety-and-health/src/lib/api.provider.ts (1 hunks)
  • libs/clients/aircraft-registry/src/lib/apiConfig.ts (1 hunks)
  • libs/clients/housing-benefit-calculator/src/lib/housing-benefit-calculator.provider.ts (1 hunks)
  • libs/clients/icelandic-government-institution-vacancies/src/lib/apiConfig.ts (1 hunks)
  • libs/clients/icelandic-health-insurance/health-insurance/src/lib/clients-health-insurance-v2.module.ts (1 hunks)
  • libs/clients/middlewares/README.md (1 hunks)
  • libs/clients/middlewares/src/lib/createEnhancedFetch.spec.ts (1 hunks)
  • libs/clients/middlewares/src/lib/createEnhancedFetch.ts (4 hunks)
  • libs/clients/middlewares/src/lib/withCircuitBreaker.ts (1 hunks)
  • libs/clients/middlewares/src/lib/withErrorLog.ts (1 hunks)
  • libs/clients/ship-registry/src/lib/ship-registry.provider.ts (1 hunks)
Files skipped from review due to trivial changes (7)
  • libs/clients/administration-of-occupational-safety-and-health/src/lib/api.provider.ts
  • libs/clients/aircraft-registry/src/lib/apiConfig.ts
  • libs/clients/housing-benefit-calculator/src/lib/housing-benefit-calculator.provider.ts
  • libs/clients/icelandic-government-institution-vacancies/src/lib/apiConfig.ts
  • libs/clients/icelandic-health-insurance/health-insurance/src/lib/clients-health-insurance-v2.module.ts
  • libs/clients/middlewares/src/lib/createEnhancedFetch.spec.ts
  • libs/clients/ship-registry/src/lib/ship-registry.provider.ts
Additional Context Used
LanguageTool (16)
libs/clients/middlewares/README.md (16)

Near line 15: Possible missing comma found.
Context: ...pressure on the remote server. Every 30 seconds we'll allow one request through. If it'...


Near line 16: Did you mean: “By default,”?
Context: ...op of standard cache-control semantics. By default nothing is cached. - Supports our `User...


Near line 28: To form a complete sentence, be sure to include a subject.
Context: ...breaker to open. Defaults to 10000ms. Can be disabled by passing false. - `logErr...


Near line 30: It appears that a comma is missing.
Context: ...meout of 10 seconds. By passing a number you can override the idle connection timeou...


Near line 45: Possible missing article found.
Context: ...ponse: Response- The response object. Body has not been consumed. -error.problem...


Near line 47: Possible missing comma found.
Context: ...Bodywas set totrue`. Parsed JSON or string depending on the response content type....


Near line 84: Loose punctuation mark.
Context: ...n IDP. There are three modes: - token: Get non-user tokens using [Client Crede...


Near line 85: Loose punctuation mark.
Context: ...fc6749#section-1.3.4). - tokenExchange: Exchange a user-token from an Auth ob...


Near line 86: Loose punctuation mark.
Context: ...ker.ietf.org/doc/html/rfc8693). - auto: Performs token exchange if an Auth ob...


Near line 88: It appears that a comma is missing.
Context: ...wise fetches a non-user token. In both modes you specify these options: - `issuer: ...


Near line 154: Consider removing “of” to be more concise
Context: ...n though the current authentication has all of the specified scopes. Defaults to false. -...


Near line 273: Consider inserting a comma after an introductory phrase for better readability.
Context: ...ute old. - If it's less than 1 day old. In this case it will immediately update the cache in...


Near line 278: Possible missing comma found.
Context: ...e an authorization headers need special consideration since those won't be stored by default....


Near line 296: The abbreviation “e.g.” (= for example) requires two periods.
Context: ...case, you need to pass a User object (eg from [@currentuser](../../auth-nest-too...


Near line 312: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...auth` argument, then a warning is logged and the cache is disabled. {% endhint %} I...


Near line 337: When ‘API-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...unction should be overridden to add the API specific header keys. ##### Example of overridi...

Path-based Instructions (4)
libs/clients/middlewares/src/lib/withCircuitBreaker.ts (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
libs/clients/middlewares/src/lib/withErrorLog.ts (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
libs/clients/middlewares/src/lib/createEnhancedFetch.ts (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
libs/clients/middlewares/README.md (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
Additional comments not posted (3)
libs/clients/middlewares/src/lib/withCircuitBreaker.ts (1)

20-24: LGTM! The error filter logic correctly aligns with the objective of not treating 4xx responses as errors.

libs/clients/middlewares/src/lib/withErrorLog.ts (1)

19-19: LGTM! The log level logic correctly aligns with the objective of logging 4xx responses as warnings.

libs/clients/middlewares/src/lib/createEnhancedFetch.ts (1)

Line range hint 33-33: LGTM! The removal of the treat400ResponsesAsErrors option aligns with the objective of not treating 4xx responses as errors.

libs/clients/middlewares/README.md Show resolved Hide resolved
@kodiakhq kodiakhq bot merged commit c4adeb9 into main May 22, 2024
230 checks passed
@kodiakhq kodiakhq bot deleted the feat/no-4xx-errors branch May 22, 2024 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecated:automerge (Disabled) Merge this PR as soon as all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants