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 8 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
7 changes: 3 additions & 4 deletions webui/src/lib/api/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -671,9 +671,8 @@ export const uploadWithProgress = (url, file, method = 'POST', onProgress = null
status: xhr.status,
body: xhr.responseText,
contentType: xhr.getResponseHeader('Content-Type'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this left?
Doesn't rawHeaders include this one as well?

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('abort', () => reject(new Error('Upload Aborted')));
Expand Down Expand Up @@ -1125,7 +1124,7 @@ class Statistics {

class Staging {
async get(repoId, branchId, path, presign = false) {
const query = qs({path, presign});
const query = qs({path, presign});
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation looks off

const response = await apiRequest(`/repositories/${encodeURIComponent(repoId)}/branches/${encodeURIComponent(branchId)}/staging/backing?` + query, {
method: 'GET'
});
Expand Down
65 changes: 35 additions & 30 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,47 @@ 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);
function extractChecksumFromResponse(rawHeaders) {
const headersString = typeof rawHeaders === 'string' ? rawHeaders : rawHeaders.toString();
const cleanedHeadersString = headersString.trim();
const headerLines = cleanedHeadersString.split('\n');
const parsedHeaders = {};

headerLines.forEach((line) => {
const [key, value] = line.split(':', 2).map((part) => part.trim());
Copy link
Contributor

Choose a reason for hiding this comment

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

Splitting this way means that if there's a : in the value of the line (the value of the header) it won't appear in the array. It's possible that it will never happen but I don't really know. If you want to make sure that the array will be split into a header name and a value (which might include a :) you can use something like:

let [key, ...value] = line.split(':')
value = value.join(':')

Copy link
Contributor

Choose a reason for hiding this comment

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

In addition, it would be great if you could extract the headers map creation out (it could be used on its own), and call it from this function, or even better, call it from the uploadFile function, and pass it to this function.

if (key && value) {
parsedHeaders[key.toLowerCase()] = value;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

More of a styling suggestion to initialize the parsedHeaders variable with data in a more functional way:

Suggested change
const parsedHeaders = {};
headerLines.forEach((line) => {
const [key, value] = line.split(':', 2).map((part) => part.trim());
if (key && value) {
parsedHeaders[key.toLowerCase()] = value;
}
const parsedHeaders = headerLines.reduce((acc, line) => {
const [key, value] = line.split(':', 2).map((part) => part.trim());
if (key && value) {
acc[key.toLowerCase()] = value;
}
return acc;
}, {});

return result;
}
});

if (response.etag) {
// drop any quote and space
return response.etag.replace(/[" ]+/g, "");
if (parsedHeaders['content-md5']) {
return parsedHeaders['content-md5'];
Jonathan-Rosenberg marked this conversation as resolved.
Show resolved Hide resolved
}
// fallback to ETag
if (parsedHeaders['etag']) {
const cleanedEtag = parsedHeaders['etag'].replace(/"/g, '');
return cleanedEtag;
Jonathan-Rosenberg marked this conversation as resolved.
Show resolved Hide resolved
}
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);
const checksum = extractChecksumFromResponse(uploadResponse.rawHeaders);
if (uploadResponse.status >= 400) {
throw new Error(`Error uploading file: HTTP ${uploadResponse.status}`);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const checksum = extractChecksumFromResponse(uploadResponse.rawHeaders);
if (uploadResponse.status >= 400) {
throw new Error(`Error uploading file: HTTP ${uploadResponse.status}`);
}
if (uploadResponse.status >= 400) {
throw new Error(`Error uploading file: HTTP ${uploadResponse.status}`);
}
const checksum = extractChecksumFromResponse(uploadResponse.rawHeaders);

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