Handle 404 errors in QueryComboBox

[sharadsw]

Fixes #5138

Adding a strict param to rgetPromise like fetchResource does to pass 404 as an expected error and avoiding an uncaught exception

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

Lines 42 to 63 in 6a364e0

	export const fetchResource = async <
	TABLE_NAME extends keyof Tables,
	SCHEMA extends Tables[TABLE_NAME],
	STRICT extends boolean = true
	>(
	tableName: TABLE_NAME,
	id: number,
	// @ts-expect-error Whether to trigger 404 on resource not found
	strict: STRICT = true
	): Promise<
	SerializedResource<SCHEMA> | (STRICT extends true ? never : undefined)
	> =>
	ajax<SerializedRecord<SCHEMA>>(
	`/api/specify/${tableName.toLowerCase()}/${id}/`,
	{
	headers: { Accept: 'application/json' },
	expectedErrors: strict ? undefined : [Http.NOT_FOUND],
	}
	).then(({ data: record, status }) =>
	status === Http.NOT_FOUND ? undefined! : serializeResource(record)
	);

Checklist

• Self-review the PR after opening it to make sure the changes look good
  and self-explanatory (or properly documented)
• Add automated tests
• Add relevant issue to release milestone

Testing instructions

[sharadsw]

Here's what I have so far @maxpatiiuk. Would appreciate some pointers

[sharadsw]

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.

[maxpatiiuk]

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

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

[sharadsw]

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

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

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

[sharadsw]

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)) {

[maxpatiiuk]

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()

[maxpatiiuk]

For future reference, here is how you can temporary disable MySQL integrity protection:

https://stackoverflow.com/a/15501754/8584605

useful for testing 404 errors on related records

[maxpatiiuk]

the code looks good,
and so this might now be ready for testing

though, could you please leave a comment on the related issue like mentioned in https://github.com/specify/specify7/pull/5139/files#r1692360885 ?