Skip to content

Commit

Permalink
Post reviews fixups
Browse files Browse the repository at this point in the history
  • Loading branch information
benzekrimaha committed Nov 19, 2024
1 parent 751140a commit 9cc8a28
Show file tree
Hide file tree
Showing 20 changed files with 1,523 additions and 1,529 deletions.
6 changes: 4 additions & 2 deletions .github/workflows/tests.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ jobs:
uses: actions/checkout@v4
- uses: actions/setup-node@v4
with:
node-version: '20'
node-version: '22'
cache: 'yarn'
- name: install dependencies
run: yarn install --frozen-lockfile --prefer-offline --network-concurrency 1
Expand All @@ -43,6 +43,8 @@ jobs:
sudo sh -c "echo '127.0.0.1 testrequestbucket.localhost' >> /etc/hosts"
- name: test and coverage
run: yarn --silent coverage
env:
NODE_OPTIONS: "--tls-max-v1.2"
- name: run functional tests
run: yarn ft_test
- uses: codecov/codecov-action@v4
Expand All @@ -62,7 +64,7 @@ jobs:
- name: Install NodeJS
uses: actions/setup-node@v4
with:
node-version: '20'
node-version: '22'
cache: yarn
- name: Install dependencies
run: yarn install --frozen-lockfile --prefer-offline
Expand Down
2 changes: 1 addition & 1 deletion lib/algos/list/delimiter.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
'use strict';
'use strict';

const Extension = require('./Extension').default;
const { inc, listingParamsMasterKeysV0ToV1,
Expand Down
3 changes: 1 addition & 2 deletions lib/models/LifecycleConfiguration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,6 @@ export default class LifecycleConfiguration {
break;
} else {
const propName = prop.propName;

delete prop.propName;
if (prop[propName] !== undefined) {
ruleObj[propName] = prop[propName];
Expand Down Expand Up @@ -675,7 +674,7 @@ export default class LifecycleConfiguration {
':([0-5][0-9])' + // Second
'(\\.[0-9]+)?' + // Fractional second
'(Z|[+-][01][0-9]:[0-5][0-9])?$', // Timezone
'g'
'g',
);
const matches = [...date.matchAll(isoRegex)];
if (matches.length !== 1) {
Expand Down
35 changes: 19 additions & 16 deletions lib/models/ObjectMD.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,15 @@ export type ReplicationInfo = {
role: string;
storageType: string;
dataStoreVersionId: string;
isNFS: boolean | null;
isNFS?: boolean;
};

export type ObjectMDData = {
'owner-display-name': string;
'owner-id': string;
'cache-control': string;
'content-disposition': string;
'content-language': string;
'content-language'?: string;
'content-encoding': string;
'creation-time'?: string;
'last-modified'?: string;
Expand Down Expand Up @@ -84,12 +84,12 @@ export type ObjectMDData = {
// Used for keeping object metadata in the oplog event
// In case of a deletion the flag is first updated before
// deleting the object
deleted: boolean;
deleted?: boolean;
// PHD flag indicates whether the object is a temporary placeholder.
// This is the case when the latest version of an object gets deleted
// the master is set as a placeholder and gets updated with the new latest
// version data after a certain amount of time.
isPHD: boolean;
isPHD?: boolean;
};

/**
Expand Down Expand Up @@ -183,7 +183,7 @@ export default class ObjectMD {
'content-length': 0,
'content-type': '',
'content-md5': '',
'content-language': '',
'content-language': undefined,
'creation-time': undefined,
// simple/no version. will expand once object versioning is
// introduced
Expand All @@ -197,7 +197,7 @@ export default class ObjectMD {
'x-amz-server-side-encryption-aws-kms-key-id': '',
'x-amz-server-side-encryption-customer-algorithm': '',
'x-amz-website-redirect-location': '',
'x-amz-scal-transition-in-progress': false,
'x-amz-scal-transition-in-progress': undefined,
'acl': {
Canned: 'private',
FULL_CONTROL: [],
Expand Down Expand Up @@ -227,12 +227,12 @@ export default class ObjectMD {
role: '',
storageType: '',
dataStoreVersionId: '',
isNFS: null,
isNFS: undefined,
},
'dataStoreName': '',
'originOp': '',
'deleted': false,
'isPHD': false,
'deleted': undefined,
'isPHD': undefined,
};
}

Expand All @@ -250,7 +250,10 @@ export default class ObjectMD {
// objMd is a new JS object created for the purpose, it's safe
// to just assign its top-level properties.

Object.assign(this._data, objMd);
const { replicationInfo, ...md } = objMd as ObjectMDData;
Object.assign(this._data, md);
// keep default values inside replicationInfo
Object.assign(this._data.replicationInfo, replicationInfo);
this._convertToLatestModel();
}

Expand Down Expand Up @@ -479,7 +482,7 @@ export default class ObjectMD {
* @return content-language
*/
getContentLanguage() {
return this._data['content-language'];
return this._data['content-language'] || '';
}

/**
Expand Down Expand Up @@ -677,7 +680,7 @@ export default class ObjectMD {
* @return True if transition is in progress, false otherwise
*/
getTransitionInProgress() {
return this._data['x-amz-scal-transition-in-progress'];
return !!this._data['x-amz-scal-transition-in-progress'];
}

/**
Expand Down Expand Up @@ -1065,7 +1068,7 @@ export default class ObjectMD {
role,
storageType: storageType || '',
dataStoreVersionId: dataStoreVersionId || '',
isNFS: isNFS || null,
isNFS,
};
return this;
}
Expand Down Expand Up @@ -1099,7 +1102,7 @@ export default class ObjectMD {
* @return Whether replication from an NFS bucket
*/
getReplicationIsNFS() {
return this._data.replicationInfo.isNFS;
return !!this._data.replicationInfo.isNFS;
}

setReplicationSiteStatus(site: string, status: string) {
Expand Down Expand Up @@ -1461,7 +1464,7 @@ export default class ObjectMD {
* @return {Boolean}
*/
getDeleted() {
return this._data.deleted;
return !!this._data.deleted;
}

/**
Expand All @@ -1479,6 +1482,6 @@ export default class ObjectMD {
* @return {Boolean}
*/
getIsPHD() {
return this._data.isPHD;
return !!this._data.isPHD;
}
}
1 change: 0 additions & 1 deletion lib/models/ObjectMDAmzRestore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ export default class ObjectMDAmzRestore {
*/
static isValid(data: { 'ongoing-request': boolean; 'expiry-date': Date | string }) {
try {

new ObjectMDAmzRestore(data['ongoing-request'], data['expiry-date']);
return true;
} catch {
Expand Down
1 change: 0 additions & 1 deletion lib/models/ObjectMDArchive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ export default class ObjectMDArchive {
restoreWillExpireAt?: Date;
}) {
try {

new ObjectMDArchive(
data.archiveInfo,
data.restoreRequestedAt,
Expand Down
4 changes: 2 additions & 2 deletions lib/models/ReplicationConfiguration.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import assert from 'assert';
import UUID from 'uuid';
const { v4: uuid } = require('uuid');

import { RequestLogger } from 'werelogs';

Expand Down Expand Up @@ -157,7 +157,7 @@ export default class ReplicationConfiguration {
obj.id =
rule.ID && rule.ID[0] !== ''
? rule.ID[0]
: Buffer.from(UUID.v4()).toString('base64');
: Buffer.from(uuid()).toString('base64');

Check warning on line 160 in lib/models/ReplicationConfiguration.ts

View check run for this annotation

Codecov / codecov/patch

lib/models/ReplicationConfiguration.ts#L160

Added line #L160 was not covered by tests
// StorageClass is an optional property.
if (rule.Destination[0].StorageClass) {
obj.storageClass = rule.Destination[0].StorageClass[0];
Expand Down
5 changes: 4 additions & 1 deletion lib/policyEvaluator/utils/actionMaps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ const actionMonitoringMapS3 = {
serviceGet: 'ListBuckets',
bucketGetQuota: 'GetBucketQuota',
bucketUpdateQuota: 'UpdateBucketQuota',
bucketDeleteQuota: 'DeleteBucketQuota',
bucketDeleteQuota: 'DeleteBucketQuota',
};

const actionMapAccountQuotas = {
Expand Down Expand Up @@ -233,6 +233,9 @@ const actionMapScuba = {
AdminStopIngest: 'scuba:AdminStopIngest',
AdminReadRaftCseq: 'scuba:AdminReadRaftCseq',
AdminTriggerRepair: 'scuba:AdminTriggerRepair',
AdminStartDownsample: 'scuba:AdminStartDownsample',
AdminStopDownsample: 'scuba:AdminStopDownsample',
AdminTriggerDownsample: 'scuba:AdminTriggerDownsample',
};

export {
Expand Down
2 changes: 1 addition & 1 deletion lib/policyEvaluator/utils/conditions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ export function convertConditionOperator(operator: string): boolean {
} else {
return policyValRegex(key);
}
return true;
return undefined;

Check warning on line 280 in lib/policyEvaluator/utils/conditions.ts

View check run for this annotation

Codecov / codecov/patch

lib/policyEvaluator/utils/conditions.ts#L280

Added line #L280 was not covered by tests
},
StringNotLike: function stringNotLike(key: string, value: string[]) {
return !operatorMap.StringLike(key, value);
Expand Down
1 change: 0 additions & 1 deletion lib/policyEvaluator/utils/wildcards.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ export const handleWildcards = (string: string) => {
// Escape all regExp special characters
// Then replace the AWS special characters with regExp equivalents
const regExStr = string.replace(/[\\^$*+?.()|[\]{}]/g, '\\$&').replace(

/(\\\*)|(\\\?)|(\\\$\\\{\\\*\\\})|(\\\$\\\{\\\?\\\})|(\\\$\\\{\\\$\\\})/g,
characterMap
);
Expand Down
1 change: 0 additions & 1 deletion lib/s3middleware/processMpuParts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ export function generateMpuPartStorageInfo(filteredPartList: any[]) {
cipheredDataKey: location.sseCipheredDataKey,
};
dataLocations.push(pieceRetrievalInfo);

calculatedSize += pieceSize;
}
});
Expand Down
4 changes: 3 additions & 1 deletion lib/s3middleware/userMetadata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ export function getMetaHeaders(headers: http.IncomingHttpHeaders) {
const rawHeaders = Object.entries(headers);
const filtered = rawHeaders.filter(([k]) => k.startsWith('x-amz-meta-'));
const totalLength = filtered.reduce((length, [k, v]) => {
if (!v) {return length;}
if (!v) {
return length;

Check warning on line 15 in lib/s3middleware/userMetadata.ts

View check run for this annotation

Codecov / codecov/patch

lib/s3middleware/userMetadata.ts#L15

Added line #L15 was not covered by tests
}
return length + k.length + v.toString().length;
}, 0);
if (totalLength <= constants.maximumMetaHeadersSize) {
Expand Down
2 changes: 1 addition & 1 deletion lib/versioning/Version.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ export class Version {
const needComma = stringifiedObject.charAt(index) !== '{';
return (
`${stringifiedObject.slice(0, stringifiedObject.length - 1)}${
needComma ? ',' : ''
needComma ? ',' : ''
}"${key}":"${value}"}`
);
}
Expand Down
5 changes: 3 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
"dependencies": {
"@azure/identity": "^4.5.0",
"@azure/storage-blob": "^12.25.0",
"@eslint/plugin-kit": "^0.2.3",
"@js-sdsl/ordered-set": "^4.4.2",
"@types/async": "^3.2.24",
"@types/utf8": "^3.0.3",
Expand Down Expand Up @@ -84,11 +85,11 @@
"lint": "eslint $(git ls-files '*ts' ' *.js')",
"lint_md": "mdlint $(git ls-files '*.md')",
"lint_yml": "yamllint $(git ls-files '*.yml')",
"test": "export NODE_OPTIONS=\"--tls-max-v1.2\" && jest tests/unit --detectOpenHandles",
"test": "jest tests/unit --detectOpenHandles",
"build": "tsc",
"prepare": "yarn build",
"ft_test": "jest tests/functional --testTimeout=120000 --forceExit",
"coverage": "export NODE_OPTIONS=\"--tls-max-v1.2\" && nyc --clean jest tests --coverage --testTimeout=120000 --forceExit",
"coverage": "nyc --clean jest tests --coverage --testTimeout=120000 --forceExit",
"build_doc": "cd documentation/listingAlgos/pics; dot -Tsvg delimiterStateChart.dot > delimiterStateChart.svg; dot -Tsvg delimiterMasterV0StateChart.dot > delimiterMasterV0StateChart.svg; dot -Tsvg delimiterVersionsStateChart.dot > delimiterVersionsStateChart.svg"
},
"private": true,
Expand Down
2 changes: 1 addition & 1 deletion tests/functional/clustering/ClusterRPC.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ function runTest(testUrl, cb) {
.on('error', err => cb(err));
}

describe.only('ClusterRPC', () => {
describe('ClusterRPC', () => {
beforeAll(done => startTestServer(done));
afterAll(done => stopTestServer(done));

Expand Down
2 changes: 1 addition & 1 deletion tests/functional/metadata/mongodb/listObject.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,7 @@ describe('MongoClientInterface::metadata.listObject', () => {
// In v1 case, the skip algorithm will trigger a recursive
// call of the internal listing function
// that should, upon completion, call the destroy method
assert(destroyStub.callCount === 3, 'Destroy should have been called 3 times');
assert(destroyStub.callCount >= 3, 'Destroy should have been called at least 3 times');
} else {
assert(destroyStub.callCount === 2, 'Destroy should have been called once');
}
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/algos/list/delimiterMaster.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
'use strict';
'use strict';

const assert = require('assert');

Expand Down
6 changes: 3 additions & 3 deletions tests/unit/models/ObjectMD.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ describe('ObjectMD class setters/getters', () => {
role: '',
storageType: '',
dataStoreVersionId: '',
isNFS: null,
isNFS: undefined,
}],
['ReplicationInfo', {
status: 'PENDING',
Expand All @@ -114,10 +114,10 @@ describe('ObjectMD class setters/getters', () => {
'arn:aws:iam::account-id:role/dest-resource',
storageType: 'aws_s3',
dataStoreVersionId: '',
isNFS: null,
isNFS: undefined,
}],
['DataStoreName', null, ''],
['ReplicationIsNFS', null, null],
['ReplicationIsNFS', null, false],
['ReplicationIsNFS', true],
['AzureInfo', {
containerPublicAccess: 'container',
Expand Down
Loading

0 comments on commit 9cc8a28

Please sign in to comment.