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.

Sign up for GitHub

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

Jump to bottom

Handle 404 errors in QueryComboBox #5139

Draft
wants to merge 10 commits into
base: production
Choose a base branch
from
Draft

Handle 404 errors in QueryComboBox #5139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8b666b5
Add strict mode to rgetPromise and resource fetch
sharadsw Jul 24, 2024
6939172
Catch errors in QCBX
sharadsw Jul 24, 2024
9faedc7
docs(ajax): improve comments documentation
maxpatiiuk Jul 25, 2024
948c118
conditionally call hijackBackbone
sharadsw Jul 30, 2024
d53d25f
throw error on an expected 404 error
sharadsw Jul 30, 2024
24eebd4
Merge remote-tracking branch 'origin/issue-5138' into issue-5138
sharadsw Jul 30, 2024
c18eefe
Merge branch 'production' into issue-5138
sharadsw Jul 30, 2024
b6ee081
fix fetch call
sharadsw Jul 31, 2024
404e701
Lint code with ESLint and Prettier
sharadsw Jul 31, 2024
5f41486
Merge branch 'production' into issue-5138
grantfitzsimmons Aug 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion specifyweb/frontend/js_src/lib/components/DataModel/legacyTypes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,8 @@ export type SpecifyResource<SCHEMA extends AnySchema> = {
SCHEMA['toOneIndependent'])[FIELD_NAME]
>(
fieldName: FIELD_NAME,
prePopulate?: boolean
prePopulate?: boolean,
strict?: boolean
): readonly [VALUE] extends readonly [never]
? never
: Promise<
Expand All @@ -98,6 +99,7 @@ export type SpecifyResource<SCHEMA extends AnySchema> = {
options?: {
readonly prePop?: boolean;
readonly noBusinessRules?: boolean;
readonly strict?: boolean;
}
): readonly [VALUE] extends readonly [never]
? never
Expand Down
23 changes: 14 additions & 9 deletions specifyweb/frontend/js_src/lib/components/DataModel/resourceApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,9 +511,9 @@ export const ResourceBase = Backbone.Model.extend({
* REFACTOR: remove the need for this
* Like "rget", but returns native promise
*/
async rgetPromise(fieldName, prePop = true) {
async rgetPromise(fieldName, prePop = true, strict = true) {
return (
this.getRelated(fieldName, { prePop })
this.getRelated(fieldName, { prePop, strict })
// GetRelated may return either undefined or null (yuk)
.then((data) => (data === undefined ? null : data))
);
Expand All @@ -532,7 +532,7 @@ export const ResourceBase = Backbone.Model.extend({
: fieldName.split(backboneFieldSeparator);

// First make sure we actually have this object.
return this.fetch()
return this.fetch(options)
.then((_this) => _this._rget(path, options))
.then((value) => {
/*
Expand All @@ -544,7 +544,8 @@ export const ResourceBase = Backbone.Model.extend({
if (!value) return value; // Ok if the related resource doesn't exist
else if (typeof value.fetchIfNotPopulated === 'function')
return value.fetchIfNotPopulated();
else if (typeof value.fetch === 'function') return value.fetch();
else if (typeof value.fetch === 'function')
return value.fetch(options);
}
return value;
});
Expand Down Expand Up @@ -760,14 +761,18 @@ export const ResourceBase = Backbone.Model.extend({
)
return this;
else if (this._fetch) return this._fetch;
else
return (this._fetch = Backbone.Model.prototype.fetch
.call(this, options)
.then(() => {
else {
const fetchCallback = () =>
Backbone.Model.prototype.fetch.call(this, options).then(() => {
this._fetch = null;
// BUG: consider doing this.needsSaved=false here
return this;
}));
});
return (this._fetch =
options === undefined || options.strict
? fetchCallback()
: hijackBackboneAjax([Http.NOT_FOUND], fetchCallback));
}
},
parse(_resp) {
// Since we are putting in data, the resource in now populated
Expand Down
51 changes: 32 additions & 19 deletions specifyweb/frontend/js_src/lib/components/QueryComboBox/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import type { State } from 'typesafe-reducer';
import { useAsyncState } from '../../hooks/useAsyncState';
import { useResourceValue } from '../../hooks/useResourceValue';
import { commonText } from '../../localization/common';
import { formsText } from '../../localization/forms';
import { userText } from '../../localization/user';
import { f } from '../../utils/functools';
import { getValidationAttributes } from '../../utils/parser/definitions';
Expand Down Expand Up @@ -119,7 +120,7 @@ export function QueryComboBox({
treeData !== undefined &&
collectionRelationships !== undefined &&
typeSearch !== undefined;
const { value, updateValue, validationRef, inputRef, parser } =
const { value, updateValue, validationRef, inputRef, parser, setValidation } =
useResourceValue(resource, field, undefined);

/**
Expand Down Expand Up @@ -171,30 +172,42 @@ export function QueryComboBox({
*/
field.isDependent())
? resource
.rgetPromise<string, AnySchema>(field.name)
.then(async (resource) =>
resource === undefined || resource === null
? {
label: localized(''),
resource: undefined,
}
: (value === formattedRef.current?.value &&
.rgetPromise<string, AnySchema>(field.name, true, false)
.then(async (resource) => {
setValidation([]);
if (resource === undefined || resource === null) {
return {
label: localized(''),
resource: undefined,
};
} else {
const formatted =
value === formattedRef.current?.value &&
typeof formattedRef.current === 'object'
? Promise.resolve(formattedRef.current.formatted)
: format(
? await Promise.resolve(formattedRef.current.formatted)
: await format(
resource,
typeof typeSearch === 'object'
? typeSearch.formatter
: undefined,
true
)
).then((formatted) => ({
label:
formatted ??
naiveFormatter(field.relatedTable.label, resource.id),
resource,
}))
)
);

return {
label:
formatted ??
naiveFormatter(field.relatedTable.label, resource.id),
resource,
};
}
})
.catch((_) => {
setValidation([formsText.invalidValue()]);
return {
label: localized(''),
resource: undefined,
};
})
Comment on lines +204 to +210
Copy link
Contributor Author

@sharadsw sharadsw Jul 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

As of now, the code never reaches this catch block as the 404 response is not considered to be an error. There is no 404 dialog in RSS Export Feed anymore (context: #4955) and the QCBX instead displays the formatted invalid resource and pretends it as valid.

We can hit the catch block if we also throw an error when status === Http.NOT_FOUND in line 66 over here:

specify7/specifyweb/frontend/js_src/lib/utils/ajax/backboneAjax.ts

Lines 64 to 69 in 6a364e0

.then(({ data, status }) => {
requestCallbackCopy?.(status);
if (status === Http.CONFLICT) throw new Error(data);
else if (typeof request.success === 'function')
request.success(data, 'success', undefined as never);
})

In which case, the error gets caught in QCBX, value gets cleared and a save blocker is set but it does not get detected as a change being made and so the save button is not enabled and the user won't know something changed. The save blocker is not visible until you click on the field.

Copy link
Member

Choose a reason for hiding this comment

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

after adding hijackBackboneAjax like described in #5139 (comment), it looks like the save blocker is set correctly and the save buttons turn red:

Screenshot 2024-07-24 at 17 17 51

could you verify if you also get this behavior after making that change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, save does get blocked correctly in the CO form. However, in a prod environment the page redirects to a 404 page, which doesn't solve the issue in #4955.

Save blocker still doesn't work correctly in RSS export feed although the values did get cleared
image
and the blocker is only visible after clicking on the field --- but it doesn't matter as in prod the page will still redirect to 404
image

Making the change here: #5139 (comment) along with wrapping hijackBackboneAjax over the resource fetch directly in resourceApi.ts helps avoid the 404

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe if we use hijackBackboneAjax higher up the call stack as opposed to wrapping it over resource.fetch(), it gets set as undefined before all chained promises are resolved

specify7/specifyweb/frontend/js_src/lib/utils/ajax/backboneAjax.ts

Line 29 in 8d0052c

expectedErrors = undefined;

and then the expectedErrors over here is an empty list instead of [404] and so it gets treated as an uncaught error

specify7/specifyweb/frontend/js_src/lib/utils/ajax/response.ts

Lines 11 to 26 in 8d0052c

export function handleAjaxResponse<RESPONSE_TYPE = string>({
expectedErrors,
accept,
response,
errorMode,
text,
}: {
readonly expectedErrors: RA<number>;
readonly accept: MimeType | undefined;
readonly response: Response;
readonly errorMode: AjaxErrorMode;
readonly text: string;
}): AjaxResponseObject<RESPONSE_TYPE> {
// BUG: silence all errors if the page begun reloading
try {
if (response.ok || expectedErrors.includes(response.status)) {

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, save does get blocked correctly in the CO form. However, in a prod environment the page redirects to a 404 page, which doesn't solve the issue in #4955.

ah, sorry! I forgot that the behavior is different in production

You are correct
The cause of the issue is that rget is actual awaiting two promises:

  1. Loading the resource itself (even if it's already loaded, this still creates a promise):
  2. Loading the related resource:

    specify7/specifyweb/frontend/js_src/lib/components/DataModel/resourceApi.ts

    Lines 521 to 522 in 9faedc7

    return this.fetch(options)
    .then((_this) => _this._rget(path, options))

the assumption that hijackBackboneAjax() makes is that the variables it sets (expectedErrors, among others) are going to be retrieved in the same cycle - but that is not the case as the rget has two promises, and the one we are interested in comes 2nd.

The issue is that hijackBackboneAjax() is called concurrently by multiple query combo boxes (i.e accession), and so if accession resource is fetched earlier, the hijackBackboneAjax() does cleanup afterward, unsettling expectedErrors and other variables - before the collector request was sent out with correct expectedErrors

In all other places where hijackBackboneAjax is used, it's used directly on .fetch(), so this issue is not present.

two options:

  • 1st (not great): make rget only call fetch() if resource is not yet fetched. not great as then this bug would still be present when working with resource that is not yet fetched
  • 2nd (close to what you have right now): hijackBackboneAjax itself is a hack. relying on global variables is not great, and even worse when working with async actions. a better solution is to pass fetch options along with requests that do the fetching.

we don't want to do too many changes to resourceApi.ts as we are hoping to do major refactor to it soon, but in an ideal world the change you made with introducing the "options" parameter would be made for all methods in resourceApi that make network requests.

for now, could you please make the following changes:

  • only call hijackBackboneAjax() in .fetch() if some options were actually provided - a performance optimization since .fetch() is very much on a hot path
  • post a comment in Migrate off backbone.js, jQuery and underscore.js #4286 summarising the issue ("we need more control over how resourceApi makes network requests ...") and say that the new resourceApi API we will design should have fetching options on all async methods. since the new API will be using ajax() directly, it will be able to easily pass on the options, without the need for hacky hijackBackboneAjax()

: { label: userText.noPermission(), resource: undefined },
[version, value, resource, field, typeSearch]
),
Expand Down
3 changes: 2 additions & 1 deletion specifyweb/frontend/js_src/lib/utils/ajax/backboneAjax.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Backbone } from '../../components/DataModel/backbone';
import { promiseToXhr } from '../../components/DataModel/resourceApi';
import { formatUrl } from '../../components/Router/queryString';
import { f } from '../functools';
import type { RA, ValueOf } from '../types';
import { defined } from '../types';
import { Http } from './definitions';
Expand Down Expand Up @@ -63,7 +64,7 @@ Backbone.ajax = function (request): JQueryXHR {
)
.then(({ data, status }) => {
requestCallbackCopy?.(status);
if (status === Http.CONFLICT) throw new Error(data);
if (f.includes([Http.CONFLICT, Http.NOT_FOUND], status)) throw new Error(data);
else if (typeof request.success === 'function')
request.success(data, 'success', undefined as never);
})
Expand Down
20 changes: 11 additions & 9 deletions specifyweb/frontend/js_src/lib/utils/ajax/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ export type AjaxMethod =
| 'POST'
| 'PUT';

/**
* These methods usually don't modify data, so if they fail it is not a big
* deal. If on the other than POST, PUT, DELETE, or PATCH request fails, it
* may cause data loss or data corruption
*/
const safeMethods: ReadonlySet<AjaxMethod> = new Set([
'OPTIONS',
'GET',
Expand Down Expand Up @@ -75,16 +80,13 @@ export type AjaxProps = Omit<RequestInit, 'body' | 'headers' | 'method'> & {
/**
* All front-end network requests should go through this utility.
*
* Wraps native fetch in useful helpers
* It is intended as a replacement for jQuery's ajax
*
* @remarks
* Automatically adds CSRF token to non GET requests
* Casts response to correct typescript type
* Parsers JSON and XML responses
* Handlers errors (including permission errors)
* Wraps native fetch in useful helpers:
* - Automatically adds CSRF token to non GET requests
* - Casts response to correct typescript type
* - Parses JSON and XML responses
* - Handlers errors (including permission errors)
* - Helps with request mocking in tests
*/
// "errorMode" is optional for "GET" requests
export async function ajax<RESPONSE_TYPE = string>(
url: string,
/** These options are passed directly to fetch() */
Expand Down
Loading