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

Handle missing baseVersion in entity upsert (create and update) case #1209

Merged
merged 3 commits into from
Oct 8, 2024

Conversation

ktuite
Copy link
Member

@ktuite ktuite commented Oct 2, 2024

Closes getodk/central#727

Thanks for the example test, @sadiqkhoja! I used it to debug and then made a simplified test.

When create and update are both present in an entity, Central first tries to update and then tries to create if the update failed in a certain way.

In this scenario, the update step was failing at the parsing step. The update flag in the entity system data is set to true, but there is no base version, and that causes an error. The code changes how the parsing works a little bit to not use what is written in the entity system data directly, but to use the context of whether it is currently trying to update or create. A similar change was made to the label parsing method.

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.

Before submitting this PR, please make sure you have:

  • run make test and confirmed all checks still pass OR confirm CircleCI build passes
  • verified that any code from external sources are properly credited in comments or that everything is internally sourced

@ktuite
Copy link
Member Author

ktuite commented Oct 2, 2024

Now this other test is failing... if we are parsing baseVersion in the create context instead of the update context, what should we do? In the offline entity update-as-create case, we kind of wanted to track that baseVersion. In the regular entity case, we don't expect to see a baseVersion with an entity create. We could probably always parse it (return it from extractBaseVersionFromSubmission) and then just ignore it.

  1) Offline Entities
       force-processing held submissions
         should apply an entity update as a create:
     TypeError: Cannot read properties of null (reading 'should')
      at /home/circleci/repo/test/integration/api/offline-entities.js:980:49
      at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
      at async /home/circleci/repo/test/integration/api/offline-entities.js:973:7
      at async /home/circleci/repo/test/integration/api/offline-entities.js:30:3
      at async Object.transaction (node_modules/slonik/dist/src/connectionMethods/transaction.js:22:24)
      at async Object.createConnection (node_modules/slonik/dist/src/factories/createConnection.js:97:18)

@ktuite ktuite marked this pull request as ready for review October 7, 2024 22:02
@ktuite ktuite requested review from sadiqkhoja and matthew-white and removed request for sadiqkhoja October 7, 2024 22:59
@ktuite
Copy link
Member Author

ktuite commented Oct 7, 2024

This could be reviewed by either @matthew-white or @sadiqkhoja. It could probably be done async but I'm happy to review synchronously with someone too. It shouldn't take too long to talk through it.

lib/model/query/entities.js Outdated Show resolved Hide resolved
Comment on lines 61 to 62
if (update) {
if (!baseVersion)
Copy link
Member

Choose a reason for hiding this comment

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

Could these two ifs be combined?

if (update && !baseVersion) {

Or since you also have an if below that's checking isBlank(), maybe something like this would be possible:

if (isBlank(baseVersion)) {
  if (update)
    // throw ...
  return null;
}

// (No need for an `else`, since `throw` and `return null` both end the function.)
// Check type and parseInt in both create and update case
// ...
return parseInt(entity.system.baseVersion, 10);

lib/data/entity.js Outdated Show resolved Hide resolved
test/unit/model/frames/entity.js Outdated Show resolved Hide resolved
lib/model/query/entities.js Outdated Show resolved Hide resolved
@ktuite ktuite merged commit afbd30c into master Oct 8, 2024
6 checks passed
@ktuite ktuite deleted the ktuite/create_update_bug branch October 8, 2024 20:34
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.

Unable to create entity when "create" and "update" are true
2 participants