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

feat: csv upload via be #2242

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Conversation

KishenKumarrrrr
Copy link
Contributor

Problem

GSIB users on certain networks are still unable to upload CSV files.

This PR includes a fix that uses the BE to bypass the whitelisting.

@KishenKumarrrrr KishenKumarrrrr self-assigned this Feb 13, 2024
withCredentials: false,
})
// 4. Return the etag and transactionId to the FE
const formattedEtag = removeFirstAndLastCharacter(response.headers.etag)
Copy link
Member

Choose a reason for hiding this comment

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

Why do you do this, is it because:

Removing the first and last character from an ETag is a common technique used to address caching issues. ETags are used in HTTP headers to help with web caching. By removing the first and last character, your friend might be attempting to modify the ETag in order to force a cache revalidation. This can be useful when there are issues with caching and the content is not being updated as expected.

@@ -63,19 +63,20 @@ async function getMd5(blob: Blob): Promise<string> {

export async function uploadFileWithPresignedUrl(
uploadedFile: File,
presignedUrl: string
): Promise<string> {
_presignedUrl: string // Making this unused because the endpoint below generates its own presignedUrl and uploads the file
Copy link
Member

Choose a reason for hiding this comment

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

s2pid qn: Why dowan reuse _presignedUrl?

Copy link
Member

@jia1 jia1 left a comment

Choose a reason for hiding this comment

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

LGTM except for some qns. Also understand that this fix may not be merged ultimately as the team is exploring a different solution.

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