Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Fix(search): querying event logs breaks when "id" column value > int32#63193

Merged
peterguy merged 6 commits into
mainfrom
peterguy/fix-event-id-size
Jun 11, 2024
Merged

Fix(search): querying event logs breaks when "id" column value > int32#63193
peterguy merged 6 commits into
mainfrom
peterguy/fix-event-id-size

Conversation

@peterguy

@peterguy peterguy commented Jun 10, 2024

Copy link
Copy Markdown
Contributor

When querying SearchResultsQueried events on dotcom, got the error:

{
  "errors": [
    {
      "message": "sql: Scan error on column index 0, name \"id\": converting driver.Value type int64 (\"2524326457\") to a int32: value out of range",
      "path": [
        "node",
        "eventLogs",
        "nodes"
      ]
    }
  ],
  "data": {
    "node": null
  }
}

Turns out the Event struct's ID column is int32, while the underlying database column is bigint, which corresponds to Go's int64.

Changed the Event struct's ID column to int64.

Added a test to check handling of 64-bit integers.

Test plan

Automated tests should pass.

New test should pass.

run this GraphQL query using a valid user ID (can be gotten from a currentUser query)

{ "query": "query EventLogsData($userId: ID!, $first: Int, $eventName: String!) {
    node(id: $userId) {
        ... on User {
            __typename
            eventLogs(first: $first, eventName: $eventName) {
                nodes {
                    argument
                    timestamp
                    url
                }
                pageInfo {
                    hasNextPage
                }
                totalCount
            }
        }
    }
}",
"variables": {
  "userId":"<base64-encoded user id>",
  "first":15,
  "eventName":"SearchResultsQueried"
},
"operationName": "EventLogsData"
}

Changelog

  • Address "value out of range" errors when querying the event logs

peterguy added 2 commits June 10, 2024 15:43
Because the underlying database column is `bigint`, and large enough values in it (showed up on dotcom) broke event log queries.
Seemed to affect build files not related to my changes, so might need to revert this commit?
@cla-bot cla-bot Bot added the cla-signed label Jun 10, 2024
@peterguy peterguy requested a review from a team June 10, 2024 22:57
@peterguy peterguy self-assigned this Jun 10, 2024

@camdencheek camdencheek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not a full review, but do we expose this via the GraphQL int type? IIRC, those must be int32 per GraphQL spec.

@peterguy

peterguy commented Jun 10, 2024

Copy link
Copy Markdown
Contributor Author

do we expose this via the GraphQL int type?

The ID field is not exposed in the GQL API

@camdencheek camdencheek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the fix!

While we're at it, we might want to change AfterID to be an explicit int64 since it refers to the same column

@peterguy

Copy link
Copy Markdown
Contributor Author

Hmm, good point about AfterID, @camdencheek. It appears to be used just once, and the inputs for it come from a database table whose column is defined as int.

I think I'll leave it alone for now, but create a followup issue to document it.

@camdencheek

Copy link
Copy Markdown
Member

We expect that indexer job to also be broken then, right? Because the jobs it's storing will overflow the bookmark column type?

@peterguy

peterguy commented Jun 11, 2024

Copy link
Copy Markdown
Contributor Author

that indexer job to also be broken

On certain systems, yes, it's probably broken.

@peterguy peterguy merged commit 549beac into main Jun 11, 2024
@peterguy peterguy deleted the peterguy/fix-event-id-size branch June 11, 2024 17:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants