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

fix(log): handle large payload #2740

Merged
merged 5 commits into from
Sep 18, 2024
Merged

Conversation

bodinsamuel
Copy link
Collaborator

Describe your changes

Fixes https://linear.app/nango/issue/NAN-1740/handle-large-log-payload-again

  • Handle large log payload
    I had already fixed that but because we are now logging object + message in the sdk, the check has to be done twice.
    However, I also realized we have the same issue in the { input } object when triggering an action. (it can also happen inside our own codebase but less likely)

🧪 Tests

Failing validation

It should return a number, but returns a massive string

      returnTooLarge:
        description: trigger an returnTooLarge
        output: ReturnTooLarge
        endpoint: POST /unauth/act/returnTooLarge
      
    ReturnTooLarge:
      id: number
import type { NangoSync } from "../../models";

export default async function runAction(nango: NangoSync,): Promise<{ id: string }> {
  return {
    id: "a".repeat(10000000),   };
}

@bodinsamuel bodinsamuel self-assigned this Sep 17, 2024
Copy link

linear bot commented Sep 17, 2024

@bodinsamuel bodinsamuel requested a review from TBonnin September 17, 2024 12:16
*/
export function truncateJsonString(value: string, maxSize: number = MAX_LOG_PAYLOAD): string {
return truncateJsonPkg(value, maxSize).jsonString;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this file be called something like stringUtils.ts instead of json?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I asked myself the same question: it depends on how you interpret the name of the files: whether the input is important or the output.
This one takes a value as string but others takes Record, either they all related to json (no matter the encoding) or I move truncateJsonString in string.ts
Wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't feel strongly either side, json is not a bad name it is just a bit generic. Another option could be logFormatUtils.ts or something similar because those functions are mostly used in the context of logging. Feel free to keep it as it is though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we have many spots where json stringification could be an issue (that's why I moved it there actually); I'll leave it as is

@bodinsamuel bodinsamuel enabled auto-merge (squash) September 18, 2024 08:30
@bodinsamuel bodinsamuel merged commit 498c5df into master Sep 18, 2024
21 checks passed
@bodinsamuel bodinsamuel deleted the fix/log-handle-large-payload branch September 18, 2024 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants