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

Add missing type annotations & catch unhandled exceptions #2382

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MarkusPettersson98
Copy link

Description

This PR is just some minor code cleanups and bug fixes 🐞 🧹

Bug fixes

I found a lot of code on the following form

const apiClient = new BackendApiClient(ctx.req.headers);
const data = await apiClient.get<T>('api/getData');
if(data) {
  return { .. };
} else {
  return {
    notFound: true
  };
}

The problem with this is that we can't expect T to be a falsy value if apiClient.get is able to fetch the value. T will in practice be some kind of object, and as such the if-body will always be hit.

If get would fail to fetch the value, it will instead throw an exception. As such, the correct form would be

try {
  const apiClient = new BackendApiClient(ctx.req.headers);
  const data = await apiClient.get<T>('api/getData');
  return { .. };
} catch {
  return { 
    notFound: true
  };
}

This would correctly return { notFound: true } if data could not be fetched.

Since data is seldom used, it can often be elided and we simply execute apiClient.get<T>('api/getData') for the side effect of maybe triggering the exception.

Cleanups

Add type annotations to some of the aforementioned get-requests where they were missing. Nothing fancy

Changes

  • Catch a bunch of unhandled exceptions.
  • Properly redirect in case above exceptions are found (based on how the code looked when I started working on it)
  • Add type annotations to some get-requests.

Notes to reviewer

Unfortunately, this is a rather boring PR to review. I guess you could try to browse all affetched pages and try to fetch invalid data. Otherwise, it is a matter of reviewing this rather mechanical code transformation by hand.

Fix a subtle bug due to an incorrect control flow. The API request
would throw an error if a user lacked the correct permission to access
a people list, but the code assumed that the API would return a falsy
value instead. The fix was simply to replace the `if-else` based control
flow with a `try-catch` dito, properly redirecting a user which performs
an unauthorized get request for some specific people list.
Copy link
Member

@richardolsson richardolsson left a comment

Choose a reason for hiding this comment

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

I'm very happy that you and your critical eyes are part of this project! I think this PR contributes several improvements, but there are also some things that I believe could be improved further. Please see comments below.

Comment on lines +39 to +41
// Note: We don't actually care for the returned orgnaization, but we still want to perform
// the api request to know if this user may access this particular page.
await apiClient.get<ZetkinOrganization>(`/api/orgs/${orgId}`);
Copy link
Member

Choose a reason for hiding this comment

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

Actually, I don't think we need to make this request. The API request above (for the journey instance) will fail if the user cannot access it, and if it can access the journey instance it will also be able to access the organization.

Comment on lines +24 to +25
// Note: We don't actually care for the returned orgnaization, but we still want to perform
// the api request to know if this user may access this particular page.
Copy link
Member

Choose a reason for hiding this comment

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

This comment refers to "the returned organization", which I believe is a copy-paste error. Personally, I would remove the comment. I usually prefer to avoid comments (because they can become misleading over time if not carefully kept up to date) but I admit this is a grey area.

// Note: We don't actually care for the returned journeys or orgnaization, but we still want to perform
// the api request to know if this user may access this particular page.
await apiClient.get<ZetkinJourney[]>(`/api/orgs/${orgId}/journeys`);
await apiClient.get<ZetkinOrganization>(`/api/orgs/${orgId}`);
Copy link
Member

Choose a reason for hiding this comment

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

This request should not be necessary.

await apiClient.get<ZetkinJourney>(
`/api/orgs/${orgId}/journeys/${journeyId}`
);
await apiClient.get<ZetkinOrganization>(`/api/orgs/${orgId}`);
Copy link
Member

Choose a reason for hiding this comment

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

This request should not be necessary.

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.

2 participants