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

8158 bug UI not using presign for uploading objects #8365

Merged
merged 16 commits into from
Dec 10, 2024
Merged

8158 bug UI not using presign for uploading objects #8365

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
c9cd2f9
doesnt work
ItamarYuran Nov 11, 2024
5a10afc
was workin now it aint
ItamarYuran Nov 12, 2024
1c7a5f3
works!'
ItamarYuran Nov 12, 2024
635d4df
lets start
ItamarYuran Nov 13, 2024
4f7f211
better
ItamarYuran Nov 13, 2024
825d817
works, somehow
ItamarYuran Nov 20, 2024
4bc6cdb
looks gooood
ItamarYuran Nov 21, 2024
5934a42
classy
ItamarYuran Nov 21, 2024
c2d9149
with changes
ItamarYuran Nov 27, 2024
1429bb8
check identetion
ItamarYuran Dec 1, 2024
1bb78a7
this time baby
ItamarYuran Dec 1, 2024
f5a2762
changes
ItamarYuran Dec 8, 2024
2e739ac
maybe this time
ItamarYuran Dec 8, 2024
2b3ec1f
minor changes
ItamarYuran Dec 10, 2024
2917355
ohh come on
ItamarYuran Dec 10, 2024
e767f86
HTTP
ItamarYuran Dec 10, 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
13 changes: 8 additions & 5 deletions webui/src/lib/api/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,12 +670,15 @@ export const uploadWithProgress = (url, file, method = 'POST', onProgress = null
resolve({
status: xhr.status,
body: xhr.responseText,
contentType: xhr.getResponseHeader('Content-Type'),
Jonathan-Rosenberg marked this conversation as resolved.
Show resolved Hide resolved
etag: xhr.getResponseHeader('ETag'),
contentMD5: xhr.getResponseHeader('Content-MD5'),
Jonathan-Rosenberg marked this conversation as resolved.
Show resolved Hide resolved
})
rawHeaders: xhr.getAllResponseHeaders(), // add raw headers
});
});
xhr.addEventListener('error', () => reject(new Error('Upload Failed')));
xhr.addEventListener('error', () => resolve({
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain why is the error resolved instead of getting rejected?
If this is because the calling function doesn't catch it, then it should...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for this is that I wanted to throw the correct error. This way if there is an error due to missing CORS setting the calling func will catch it and throw suitable error. In case other piece of code uses this func and upload fails for some reason - the error will just state 'upload failed'.

Copy link
Contributor

Choose a reason for hiding this comment

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

The proper way of handling errors in Promises is using the try/catch mechanism at the calling function.
This should still reject, and the error (the object returned to the reject function) can be analyzed further by the calling function.
I would like to mention in this context that this is javascript and it's s**t.

status: xhr.status,
body: xhr.responseText,
rawHeaders: xhr.getAllResponseHeaders(),
error: new Error('Upload Failed'),
}));
xhr.addEventListener('abort', () => reject(new Error('Upload Aborted')));
xhr.open(method, url, true);
xhr.setRequestHeader('Accept', 'application/json');
Expand Down
71 changes: 40 additions & 31 deletions webui/src/pages/repositories/repository/objects.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,42 +226,51 @@ const ImportModal = ({config, repoId, referenceId, referenceType, path = '', onD
);
};

function extractChecksumFromResponse(response) {
if (response.contentMD5) {
// convert base64 to hex
const raw = atob(response.contentMD5)
let result = '';
for (let i = 0; i < raw.length; i++) {
const hex = raw.charCodeAt(i).toString(16);
result += (hex.length === 2 ? hex : '0' + hex);
}
return result;
}

if (response.etag) {
function extractChecksumFromResponse(rawHeaders) {
const headersString = typeof rawHeaders === 'string' ? rawHeaders : rawHeaders.toString();
const cleanedHeadersString = headersString.trim();
const headerLines = cleanedHeadersString.split('\n');
const parsedHeaders = headerLines.reduce((acc, line) => {
let [key, ...value] = line.split(':'); // split into key and the rest of the value
key = key.trim();
value = value.join(':').trim();
if (key && value) {
acc[key.toLowerCase()] = value;
}
return acc;
}, {});
Copy link
Contributor

Choose a reason for hiding this comment

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

Please extract these lines to a function called parseRawHeaders (or something like that), and instead of those line run:

const parsedHeaders = parseRawHeaders(rawHeaders);
if (parsedHeaders['content-md5']) {
.....

I would actually pass the extractChecksumFromResponse function the parsed headers as an argument and let it fetch the checksum from it.

if (parsedHeaders['content-md5']) {
// drop any quote and space
return response.etag.replace(/[" ]+/g, "");
return parsedHeaders['content-md5'];
Jonathan-Rosenberg marked this conversation as resolved.
Show resolved Hide resolved
}
// fallback to ETag
if (parsedHeaders['etag']) {
// drop any quote and space
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation again 😆

return parsedHeaders['etag'].replace(/"/g, '');
}
return ""
return null;
}

const uploadFile = async (config, repo, reference, path, file, onProgress) => {
const fpath = destinationPath(path, file);
if (config.pre_sign_support_ui) {
let additionalHeaders;
if (config.blockstore_type === "azure") {
additionalHeaders = { "x-ms-blob-type": "BlockBlob" }
}
const getResp = await staging.get(repo.id, reference.id, fpath, config.pre_sign_support_ui);
const uploadResponse = await uploadWithProgress(getResp.presigned_url, file, 'PUT', onProgress, additionalHeaders)
if (uploadResponse.status >= 400) {
throw new Error(`Error uploading file: HTTP ${status}`)
const uploadFile = async (config, repo, reference, path, file, onProgress) => {
const fpath = destinationPath(path, file);
if (config.pre_sign_support_ui) {
let additionalHeaders;
if (config.blockstore_type === "azure") {
additionalHeaders = { "x-ms-blob-type": "BlockBlob" }
}
const getResp = await staging.get(repo.id, reference.id, fpath, config.pre_sign_support_ui);
const uploadResponse = await uploadWithProgress(getResp.presigned_url, file, 'PUT', onProgress, additionalHeaders);
if (uploadResponse.status >= 400) {
throw new Error(`Error uploading file: HTTP ${uploadResponse.status}`);
}
const checksum = extractChecksumFromResponse(uploadResponse.rawHeaders);
if (checksum === null) {
throw new Error(`CORS settings error. Must configure "exposedHeaders" field`);
}
await staging.link(repo.id, reference.id, fpath, getResp, checksum, file.size, file.type);
} else {
await objects.upload(repo.id, reference.id, fpath, file, onProgress);
}
const checksum = extractChecksumFromResponse(uploadResponse)
await staging.link(repo.id, reference.id, fpath, getResp, checksum, file.size, file.type);
} else {
await objects.upload(repo.id, reference.id, fpath, file, onProgress);
}
};

const destinationPath = (path, file) => {
Expand Down
Loading