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

Unexpected 500 response: "...values and fields must be sorted..." #381

Closed
bickelj opened this issue May 31, 2023 · 2 comments
Closed

Unexpected 500 response: "...values and fields must be sorted..." #381

bickelj opened this issue May 31, 2023 · 2 comments
Assignees

Comments

@bickelj
Copy link
Contributor

bickelj commented May 31, 2023

This URL currently returns a 500: https://api.philanthropydatacommons.org/proposals/452?includeFieldsAndValues=true

{"name":"UnknownError","message":"Given values and fields must be sorted such that they correspond. index=87: 712 !== 706.","details":[{}]}
@bickelj bickelj self-assigned this May 31, 2023
@bickelj
Copy link
Contributor Author

bickelj commented May 31, 2023

The running version is ghcr.io/philanthropydatacommons/service:20230525-f59e112.

In proposalsHandlers.ts, these first two queries are expected to be sorted the same way such that they can be joined in software (see #198 (comment) and following).

      Promise.all([
        db.sql('proposalFieldValues.selectByProposalId', { proposalId: req.params.id }),
        db.sql('applicationFormFields.selectByProposalId', { proposalId: req.params.id }),
        db.sql('proposalVersions.selectByProposalId', { proposalId: req.params.id }),
      ]).then(([
//...
        const valuesWithFields = joinApplicationFormFieldsToProposalFieldValues(
          proposalFieldValuesQueryResult.rows,
          applicationFormFieldsQueryResult.rows,
        );

The join function does some consistency/assumption checks before joining:

export const joinApplicationFormFieldsToProposalFieldValues = (
  values: ProposalFieldValue[],
  fields: ApplicationFormField[],
): ProposalFieldValue[] => {
  const newValues = structuredClone(values);
  if (newValues.length !== fields.length) {
    // It is invalid to try to zip values and fields of unequal length.
    throw new Error(`Given arrays must be of equal length. ${values.length} !== ${fields.length}`);
  }

  return newValues.map((value, index) => {
    // Look in the fields array at the same index as the values array.
    const field = fields[index];
    if (field === undefined) {
      throw new Error(
        `Expected only defined fields and values, copiedFields[${index}] was undefined`,
      );
    }
    if (value.applicationFormFieldId !== field.id) {
      throw new Error(
        `Given values and fields must be sorted such that they correspond. index=${index}: ${value.applicationFormFieldId} !== ${field.id}.`,
      );
    }
    // When all is well, add the field to the value.
    return { ...value, applicationFormField: field };
  });
};

The queries ([aff]/selectByProposalId.sql and [pfv]/selectByProposalId.sql) appear to order by the same columns, ORDER BY pv.version DESC, pfv.position;, so it is not as if the SQL changed.

The length of the two query result sets is identical or we would have gotten Given arrays must be of equal length.

I ran the queries manually and am comparing the results.

Indeed the 88th and 89th (of 90) rows order is flipped. The proposal field value position is repeated and the ORDER BY clause is insufficient by only using position. The position may not always be the absolute order or be unique across proposal field values within a proposal version. Perhaps an additional id column is needed in that ORDER BY.

After dump and restore locally, the issue does not appear locally, which is not entirely unexpected because the issue only appeared after some time and the order if not explicit can be non-deterministic.

bickelj added a commit that referenced this issue May 31, 2023
Without this change, a non-deterministic sort order can cause an HTTP
500 response with message "...values and fields must be sorted...".

With this change, the sort order is first by position (as before) and
then also by proposal field value (pfv) ID because there can be many
pfv rows for a given application form field (aff) row. This should be
sufficient to guarantee deterministic order such that the application
join will succeed.

Issue #381 Unexpected 500 response: "values and fields must be sorted"

See also #198
and #210.
bickelj added a commit that referenced this issue Jun 1, 2023
Without this change, a non-deterministic sort order can cause an HTTP
500 response with message "...values and fields must be sorted...".

With this change, the sort order is first by position (as before) and
then also by proposal field value (pfv) ID because there can be many
pfv rows for a given application form field (aff) row. This should be
sufficient to guarantee deterministic order such that the application
join will succeed.

Issue #381 Unexpected 500 response: "values and fields must be sorted"

See also #198
and #210.
@bickelj
Copy link
Contributor Author

bickelj commented Jun 1, 2023

It appears to have fixed the issue in prod.

@bickelj bickelj closed this as completed Jun 1, 2023
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

No branches or pull requests

1 participant