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

If an instanceID is reused, duplicate entries will be shown in the submission table #608

Closed
lognaturel opened this issue Mar 12, 2024 · 5 comments · Fixed by getodk/central-backend#1129
Assignees
Labels
backend Requires a change to the API server behavior verified Behavior has been manually verified bug

Comments

@lognaturel
Copy link
Member

lognaturel commented Mar 12, 2024

Problem description

Some forms can have duplicate entries shown in the submission table. The submission number displayed on the left goes negative.

URL of the page

https://test.getodk.cloud/#/projects/40/forms/build_simple_1500033035/submissions

https://test.getodk.cloud/#/projects/149/forms/all-widgets/submissions - problems start happening at row 77, I believe

Steps to reproduce the problem

Scroll to the bottom of a submission table and keep scrolling.

Screenshot

Screenshot 2024-03-12 at 3 05 53 PM

Expected behavior

Duplicates are not shown.

Central version shown in version.txt

20dcbf4 (v2023.5.1)
ab0c8ecbf837c7e433b20c7d7d1d2955cc8df1c6 client (v2023.5.0)
983ec81e69793fdb589ffdc346a16ef977489be4 server (v2023.5.0)

Browser version

Not browser-related

Around when did you see the problem (in UTC)?

March 12 10:04pm UTC

Other notes (if any)

The most likely culprit is cursor-based pagination.

@lognaturel lognaturel added the bug label Mar 12, 2024
@matthew-white
Copy link
Member

I took a quick look at this. Some initial thoughts:

  • Looking at the all-widgets form, there don't seem to be duplicates in the first chunk of submissions (the first 250).
  • The 251st submission (the first submission of the second chunk) is row 77, which is duplicated 5 times. From a glance, it seems like that's the pattern: every submission in the second chunk looks to be duplicated 5 times.
  • Looking at the SQL, this JOIN involving $skiptoken looks like a potential cause to me. If the instance ID from the $skiptoken isn't unique (if other forms have a submission with the same instance ID), then the cursor subquery will return more than one row.
    • I think I can actually derive the number 5 from this. The instance ID of the 250th submission (row 78) is uuid:725cad91-d9c9-441e-a92f-bf0f6b38765e. There are 23 submissions on the QA server with that instance ID. However, only 5 satisfy the JOIN condition on createdAt: only 5 of the 23 were created after the 251st submission.

@matthew-white matthew-white added the backend Requires a change to the API server label Mar 13, 2024
@matthew-white
Copy link
Member

matthew-white commented Mar 13, 2024

This is only sorted of related, but even if the cursor subquery did return one row, I'm not sure about the AND in the JOIN condition: I think it could filter out submissions that it shouldn't. You could have a submission whose createdAt timestamp is slightly before the createdAt timestamp of the $skiptoken submission, yet also has a larger id (if its transaction started before the $skiptoken submission and finished after). Not sure, but I think the JOIN condition should be more like:

submissions."createdAt" < cursor."createdAt" or
(submissions."createdAt" = cursor."createdAt" and submissions.id < cursor.id)

In other words, cursor.id should only really be used for tie-breaks on createdAt.

@lognaturel lognaturel changed the title Older forms can have duplicate entries shown in submission table If an instanceID is reused, duplicate entries will be shown in the submission table Mar 19, 2024
matthew-white added a commit to getodk/central-backend that referenced this issue Apr 23, 2024
@matthew-white matthew-white added the needs testing Needs manual testing label Apr 24, 2024
matthew-white added a commit to getodk/central-backend that referenced this issue Apr 26, 2024
@matthew-white
Copy link
Member

This issue should now be fixed. @dbemke, probably the only thing that needs to be verified is that there are no negative row numbers.

I also wanted to leave a few notes (highlights in bold):

  • I mentioned above that two different forms can use the same instance ID. It's also the case that a form and its draft can use the same instance ID. Two published submissions of the same form can't use the same instance ID, but a draft submission and a published submission of the same form can. This case had the potential to cause the issue seen here.
  • With submission deletion coming soon, I wanted to mention that $skiptoken will be somewhat resilient to submission deletion. Nothing will break if the instance ID from a $skiptoken matches a deleted submission (a submission in the trash). Backend can still find the id and createdAt timestamp of the deleted submission, which is all it needs.
  • It's only once a submission is fully removed from the database that $skiptoken stops working. (That can happen today if a form is purged and will also happen in the future with submission purging.) If the instance ID from a $skiptoken doesn't match any of the form's submissions (deleted or otherwise), then the OData request will return a successful response with zero submissions. I don't know of a current use case that would cause $skiptoken to be stored for a long period (long enough for the associated submission to be deleted and purged). That said, I think there's a case to be made for returning an error if the instance ID from a $skiptoken doesn't match any submission. Maybe the OData spec has something to say about this.
  • Backend will return a 500 error if a $skiptoken isn't encoded correctly or if it contains invalid JSON.
  • Currently in production, if $skiptoken is specified without $top, and $count=true is specified, then the @odata.count property may be wrong. It will only count the submissions returned, not all submissions. I fixed this as well.
  • I confirmed that there was an issue whereby a submission would be excluded incorrectly if its createdAt timestamp was before the timestamp of the submission associated with the $skiptoken, yet its id was greater than than the id of the $skiptoken submission. That issue is now fixed.
  • I had noticed for the all-widgets form that each submission appear 5 times in a row. However, there's an issue beyond that that I don't fully understand. The all-widgets form has 327 submissions, yet I'm able to scroll well past row -7000. 7000 is much more than 327 x 5. For example, I see the instance ID uuid:c9213d8e-54de-4c8c-acb1-0ab42ba8234d on row 94, meaning that it's in the first chunk of submissions, before the instance ID from the first $skiptoken. However, I also see the same instance ID duplicated 6 times starting on row -1613, 5 times starting on row -2498, 4 times starting on row -3669, and 6 times starting on row -4663. In other words, (1) the number of duplicates isn't consistent, and (2) the OData seems to loop back on itself. In other words, I'm not sure if/when the scrolling would ever stop.
    • My best guess as to why this is happening is that there was an issue with a second query that uses $skiptoken. For OData, submissions are counted in a separate query from the main query, and that count query had the same issue with how a JOIN used $skiptoken. This doesn't affect the @odata.count property that's returned if $count=true is specified. Instead, it affects the count of remaining submissions, i.e., the count of submissions after the submission associated with the $skiptoken. That count of remaining submissions seems to be used to determine whether to return an @odata.nextLink. I think the issue caused @odata.nextLink to be returned when it shouldn't be. Frontend will continue to fetch submissions as long as an @odata.nextLink is returned.

@srujner
Copy link

srujner commented Apr 26, 2024

Tested with Success!

1 similar comment
@dbemke
Copy link

dbemke commented Apr 26, 2024

Tested with Success!

@dbemke dbemke added behavior verified Behavior has been manually verified and removed needs testing Needs manual testing labels Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Requires a change to the API server behavior verified Behavior has been manually verified bug
Projects
Status: ✅ done
Development

Successfully merging a pull request may close this issue.

4 participants