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

Disallow string downcast for groups and repeat groups #381

Closed
matthew-white opened this issue Jul 6, 2021 · 5 comments · Fixed by #765
Closed

Disallow string downcast for groups and repeat groups #381

matthew-white opened this issue Jul 6, 2021 · 5 comments · Fixed by #765
Assignees

Comments

@matthew-white
Copy link
Member

Right now, Backend will allow any field to be downcast to string. I just checked, and it looks like that includes groups and repeat groups. Is that something we want to prevent? My instinct is that it would make it easier to reason about types (and maybe also XML traversal?) if we could assume that structures never become strings.

@matthew-white
Copy link
Member Author

We had some issues creating forms and form drafts while QAing v1.3. I thought that it might be related to the new check_field_collisions trigger from 4d5619c, so we reverted that change (#406). Reopening this issue so that we can determine whether the new trigger was related to the issue creating forms/drafts. One thing to try is to explain analyze the query on the QA server.

Notes that I posted to Slack:

On Thursday, QA had a period of being unable to upload a form, but then it went away. But the issue seems to be back now [on Monday].

I took a look and also encountered a 504 when creating a new form. I also encountered a 504 when creating a form draft. However, I seem to be able to navigate throughout Frontend normally: the project and form lists both load, for example. I was also able to create an app user and a web user. This makes me think that there’s a Backend issue with form creation specifically.

Not sure this was a great idea, but I also went into node on the QA server and manually called Forms.createNew() (after setting everything up). The result has been a pending promise. So I think something about that query is taking a long time? Could it be the new version of check_field_collisions, or some other trigger?

I also queried pg_stat_activity (queries.txt)

I think the three queries at 14:38, 14:39, and 14:40 are me calling Forms.createNew() from node

The earliest query is insert into form_fields. Maybe consistent with the idea that check_field_collisions is locking things up?

I just checked pg_stat_activity again, and the first two queries have been cleared. Now the earliest query is insert into form_fields at 14:18.

Another thing that seems interesting is that I think the default value of statementTimeout in Slonik is 60 seconds. Yet clearly these queries are not being aborted.

I wonder if that’s consistent with this observation by Marzena: "I tried to add few different forms and every time I saw this error. ... When I returned to the project after a long while forms were visible!" Is somehow the request being aborted, but the query isn’t aborted, so even though it’s a 504 response, the form is ultimately created? That would definitely feel surprising.

OK, I think I’m actually seeing that behavior myself. I tried to create a new form draft a while ago, but got a 504. But now I see in the audit log that that draft was just now created.

The earliest query is now the insert into form_fields from 14:53. It continues to seem like queries are eventually being resolved, just slowly.

This issue was discussed in the QA document as part of v1.3, item 1. There, Marzena wrote:

I tried to add few different forms and every time I saw this error. I tried to add forms to the existed project and new project with the same result. It takes a long time before Central shows this error.

@ktuite
Copy link
Member

ktuite commented Jan 23, 2023

As part of the form schema migration in PR #725, I removed the trigger and replaced it with a simpler downcast checking function. If we need to add back a specific kind of downcast disallow, we can do that there.

@matthew-white
Copy link
Member Author

If we need to add back a specific kind of downcast disallow, we can do that there.

Maybe we do that as part of v2023.2 while this area is fresh in your mind? Unless you want a break from thinking about form schemas. 😅

@ktuite
Copy link
Member

ktuite commented Jan 24, 2023

2023.2 is fine! As long as it's not through the trigger! Though maybe I'd even feel brave enough to attempt to bring back the trigger if people really wanted it.

@ktuite ktuite moved this from 🕒 backlog to ✏️ in progress in ODK Central Feb 13, 2023
@github-project-automation github-project-automation bot moved this from ✏️ in progress to ✅ done in ODK Central Feb 13, 2023
@dbemke
Copy link

dbemke commented Mar 1, 2023

Tested with success!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ done
Development

Successfully merging a pull request may close this issue.

3 participants