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

Bai 1111 allow semver matching and latest semver for releases #1582

Merged
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
3ad2e86
Can now receive one release by string comparison in mongoose query
IR96334 Sep 24, 2024
1c421d7
Added basic semver range check, getters and setters for release semve…
IR96334 Oct 15, 2024
9e10ab3
Add series of queries for different types
IR96334 Oct 23, 2024
ae35fc8
Added @types/semver, small modifications to range query
IR96334 Oct 25, 2024
82bb578
Now plugs into releases endpoint instead of singular release.
IR96334 Oct 25, 2024
2d8b532
Changed package name
IR96334 Oct 25, 2024
a7b05b8
Simplified and grouped functionality. Also added primitive hyphen ran…
IR96334 Oct 28, 2024
725eb87
Fixed caret query
IR96334 Oct 29, 2024
da9110c
Added comments for each range query type for explanation
IR96334 Oct 31, 2024
6df897e
Added migrations script and updated schema getter to allow for migrat…
IR96334 Oct 31, 2024
d050140
Refactor
IR96334 Nov 11, 2024
1e56d7b
Bugfixed refactor, removed old code
IR96334 Nov 12, 2024
ce0f95e
merge main branch
IR96334 Nov 12, 2024
8e7f254
change as per PR
IR96334 Nov 18, 2024
235f1f3
Simplified code and added functionality for more queries
IR96334 Nov 19, 2024
47106bd
Simplified queries massively, now no longer overwrites previous query
IR96334 Nov 21, 2024
ffc7b03
Simplified queries further, added queryboundinterface and queue
IR96334 Nov 22, 2024
0215e0e
Removed unnecessary comments
IR96334 Nov 22, 2024
3915659
PR changes and added tests
IR96334 Nov 28, 2024
b5658be
Small test change
IR96334 Nov 28, 2024
f2ff99f
Test changes
IR96334 Nov 28, 2024
9604ff1
Trying fix for python tests
IR96334 Nov 28, 2024
f532454
Fixed smtp tests
IR96334 Nov 28, 2024
223dbad
Merge branch 'main' into BAI-1111-allow-semver-matching-and-latest-se…
IR96334 Nov 29, 2024
fcf9a0a
Fix for smtp error
IR96334 Nov 29, 2024
0bc56a7
Changed host to local
IR96334 Nov 29, 2024
84dfdac
Added mock avScanning config
ARADDCC012 Nov 29, 2024
9f6eb68
Updated smtp unit test mock config
ARADDCC012 Nov 29, 2024
d15fe16
Updated avScanning mock config ports
ARADDCC012 Nov 29, 2024
0546f95
Moved smtp test config into global mock config
ARADDCC012 Nov 29, 2024
5118315
Changed name of migration script
IR96334 Nov 29, 2024
ae80725
Changes to testing
IR96334 Dec 3, 2024
8f31592
Added more changes as per PR
IR96334 Dec 3, 2024
f8ca356
Removed error
IR96334 Dec 3, 2024
d6f486f
Fixed tests, mocking, added tests, and adjsuted getModelReleases
IR96334 Dec 4, 2024
8d0cb76
Fixed release query functionality
IR96334 Dec 5, 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
8 changes: 5 additions & 3 deletions backend/src/routes/v2/release/getReleases.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ export const getReleasesSchema = z.object({
}),
}),
query: z.object({
q: z.string().optional(),
querySemver: z.string().optional().openapi({ example: '>2.2.2' }).openapi({
description: 'Query for semver ranges, as described in https://docs.npmjs.com/cli/v6/using-npm/semver#ranges',
}),
}),
})

Expand Down Expand Up @@ -52,10 +54,10 @@ export const getReleases = [
req.audit = AuditInfo.ViewReleases
const {
params: { modelId },
query: { q },
query: { querySemver },
} = parse(req, getReleasesSchema)

const releases = await getModelReleases(req.user, modelId, q)
const releases = await getModelReleases(req.user, modelId, querySemver)
await audit.onViewReleases(req, releases)

return res.json({
Expand Down
179 changes: 81 additions & 98 deletions backend/src/services/release.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Document, Schema } from 'mongoose'
import { Schema } from 'mongoose'
import semver from 'semver'
import { Optional } from 'utility-types'

Expand Down Expand Up @@ -56,8 +56,7 @@ async function validateRelease(user: UserInterface, model: ModelDoc, release: Re
const fileNames: Array<string> = []

for (const fileId of release.fileIds) {
let file: Document<unknown, any, FileInterface> &
Omit<FileInterface & Required<{ _id: Schema.Types.ObjectId }>, never>
let file: FileInterface | (undefined & Omit<FileInterface & Required<{ _id: Schema.Types.ObjectId }>, never>)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what's happened here again. let file: FileInterfaceDoc | undefined should be the correct type

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm... fixed

try {
file = await getFileById(user, fileId)
} catch (e) {
Expand Down Expand Up @@ -198,7 +197,10 @@ export async function updateRelease(user: UserInterface, modelId: string, semver
})
}

const updatedRelease = await Release.findOneAndUpdate({ modelId, semver }, { $set: release })
const updatedRelease = await Release.findOneAndUpdate(
{ modelId, semver: semverStringToObject(semver) },
{ $set: release },
)

if (!updatedRelease) {
throw NotFound(`The requested release was not found.`, { modelId, semver })
Expand Down Expand Up @@ -239,11 +241,11 @@ export async function newReleaseComment(user: UserInterface, modelId: string, se
export async function getModelReleases(
user: UserInterface,
modelId: string,
q?: string,
querySemver?: string,
): Promise<Array<ReleaseDoc & { model: ModelInterface; files: FileInterface[] }>> {
const query = q === undefined ? { modelId } : getQuerySyntax(q, modelId)
const query = querySemver === undefined ? { modelId } : getQuerySyntax(querySemver, modelId)
const results = await Release.aggregate()
.match(query!)
.match(query)
.sort({ updatedAt: -1 })
.lookup({ from: 'v2_models', localField: 'modelId', foreignField: 'id', as: 'model' })
.append({ $set: { model: { $arrayElemAt: ['$model', 0] } } })
Expand All @@ -252,9 +254,16 @@ export async function getModelReleases(
const model = await getModelById(user, modelId)

const auths = await authorisation.releases(user, model, results, ReleaseAction.View)
return results
.filter((_, i) => auths[i].success)
.map((result) => ({ ...result, semver: semverObjectToString(result.semver) }))
return results.reduce<Array<ReleaseDoc & { model: ModelInterface; files: FileInterface[] }>>(
(updatedResults, result, index) => {
if (auths[index].success) {
updatedResults.push({ ...result, semver: semverObjectToString(result.semver) })
}

return updatedResults
},
[],
)
}

export async function getReleasesForExport(user: UserInterface, modelId: string, semvers: string[]) {
Expand Down Expand Up @@ -282,7 +291,7 @@ export async function getReleasesForExport(user: UserInterface, modelId: string,
return releases
}

export function semverStringToObject(semver: string) {
export function semverStringToObject(semver: string): SemverObject {
const vIdentifierIndex = semver.indexOf('v')
const trimmedSemver = vIdentifierIndex === -1 ? semver : semver.slice(vIdentifierIndex + 1)
const [version, metadata] = trimmedSemver.split('-')
Expand All @@ -293,7 +302,7 @@ export function semverStringToObject(semver: string) {
return { major: majorNum, minor: minorNum, patch: patchNum, ...(metadata && { metadata }) }
}

export function semverObjectToString(semver: SemverObject) {
export function semverObjectToString(semver: SemverObject): string {
if (!semver) {
return ''
}
Expand Down Expand Up @@ -330,50 +339,29 @@ function getSemverQueryBounds(querySemver: string) {
const semverRangeStandardised = semver.validRange(querySemver, { includePrerelease: false })

if (!semverRangeStandardised) {
throw BadReq(`Semver range, '${querySemver}' is invalid `)
throw BadReq(`Semver range, '${querySemver}' is invalid.`)
Copy link
Member

Choose a reason for hiding this comment

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

In general with errors, we try and avoid injecting variables into the messages. Instead you can pass in a context object containing any relevant data. Eg throw BadReq(Semver range is invalid., {semverQuery: querySemver})

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed

}

const [expressionA, expressionB] = semverRangeStandardised.split(' ')

let lowerSemver: string = '',
upperSemver: string = '',
lowerInclusivity = false,
upperInclusivity = false
let lowerInclusivity, lowerSemver, upperInclusivity, upperSemver
Copy link
Member

@ARADDCC012 ARADDCC012 Dec 3, 2024

Choose a reason for hiding this comment

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

These are all any, they should be typed. If this was a JS project I wouldn't have a problem with defining multiple vars on a single line like this, but in TS we should either be providing a default value or explicitly typing it as MyType | undefined. As a result it's much more readable to define them on separate lines.

let var1: MyType | undefined
let var: MyOtherType | undefined
// etc

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed


//If lower semver
if (expressionA.includes('>')) {
Copy link
Member

Choose a reason for hiding this comment

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

When an if statement becomes this complex it's always worth seeing if it could be simplified.
Have you considered:

const lowerInclusivity = expressionA.includes('>=')
const upperInclusivity = expressionB.includes('<=') | expressionA.includes('<=')
const lowerSemver = expressionA.replace(/[<>=]/g, '')
const upperSemver = expressionB.replace(/[<>=]/g, '') 

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, still required some if statements as expressionA can be the upperSemver as the lower/upper parts of the variables state wether they are upper or lower bounds, not first or second in query

if (expressionA.includes('=')) {
lowerSemver = expressionA.replace('>=', '')
lowerInclusivity = true
} else {
lowerSemver = expressionA.replace('>', '')
}
//do check for expressionB
if (expressionB) {
if (expressionB.includes('=')) {
upperSemver = expressionB.replace('<=', '')
upperInclusivity = true
} else {
upperSemver = expressionB.replace('<', '')
}
}
lowerInclusivity = expressionA.includes('>=')
lowerSemver = expressionA.replace(/[<>=]/g, '')
Copy link
Member

@JR40159 JR40159 Dec 2, 2024

Choose a reason for hiding this comment

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

To avoid having multiple variables for the upper and lower semvers and another set of if statements after this one, could you include .replace(/[<>=]/g, '') at the start of the function semverStringToObject() and call semverStringToObject() here? It would also mean you'd only need to have this replace in one place

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. I still believe I need all 3 replaces as I don't know whether expressionA is lower or upper.

} else {
if (expressionA.includes('=')) {
upperSemver = expressionA.replace('<=', '')
upperInclusivity = true
} else {
upperSemver = expressionA.replace('<', '')
}
//upper semver
upperInclusivity = expressionA.includes('<=')
upperSemver = expressionA.replace(/[<=]/g, '')
}

let lowerSemverObj: { metadata?: string | undefined; major: number; minor: number; patch: number } = {
major: NaN,
minor: NaN,
patch: NaN,
},
upperSemverObj: { metadata?: string | undefined; major: number; minor: number; patch: number } = {
major: NaN,
minor: NaN,
patch: NaN,
}
if (expressionB) {
upperInclusivity = expressionB.includes('<=')
upperSemver = expressionB.replace(/[<=]/g, '')
}

let lowerSemverObj, upperSemverObj
Copy link
Member

@ARADDCC012 ARADDCC012 Dec 3, 2024

Choose a reason for hiding this comment

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

If you create variables without a default value TS will treat them as any, so these should both be typed. Also defining multiple values in a single line is something we should probably avoid.

let lowerSemverObj: SemverObject | undefined
let upperSemverObj: SemverObject | undefined

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah removed the typing because I wanted it to be undefined, forgetting that I can give the possible type of undefined!

if (lowerSemver) {
lowerSemverObj = semverStringToObject(lowerSemver)
}
Expand All @@ -385,58 +373,53 @@ function getSemverQueryBounds(querySemver: string) {
return { lowerSemverObj, upperSemverObj, lowerInclusivity, upperInclusivity }
}

function getQuerySyntax(querySemver: string | undefined, modelID: string) {
if (querySemver === undefined) {
return {
modelId: modelID,
}
}
interface QueryBoundInterface {
$or: (
| {
'semver.major':
| {
$lte: number
}
| {
$gte: number
}
'semver.minor':
| {
$lte: number
}
| { $gte: number }
'semver.patch':
| {
$gte: number
}
| { $gt: number }
| { $lte: number }
| { $lt: number }
}
| {
'semver.major':
| {
$gt: number
}
| { $lt: number }
}
| {
'semver.major':
| {
$gte: number
}
| { $lte: number }
'semver.minor':
| {
$gt: number
}
| { $lt: number }
}
)[]
}

export function getQuerySyntax(querySemver: string, modelID: string) {
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't need to export this? To make it clear what this function is doing, could we change the name to something like convertSemverQueryToMongoQuery()

Copy link
Member Author

Choose a reason for hiding this comment

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

I have changed the name. I have exported as I test it in release.spec.ts test file.

const { lowerSemverObj, upperSemverObj, lowerInclusivity, upperInclusivity } = getSemverQueryBounds(querySemver)
interface QueryBoundInterface {
$or: (
| {
'semver.major':
| {
$lte: number
}
| {
$gte: number
}
'semver.minor':
| {
$lte: number
}
| { $gte: number }
'semver.patch':
| {
$gte: number
}
| { $gt: number }
| { $lte: number }
| { $lt: number }
}
| {
'semver.major':
| {
$gt: number
}
| { $lt: number }
}
| {
'semver.major':
| {
$gte: number
}
| { $lte: number }
'semver.minor':
| {
$gt: number
}
| { $lt: number }
}
)[]
}

const queryQueue: QueryBoundInterface[] = []

Expand Down
25 changes: 25 additions & 0 deletions backend/src/utils/__mocks__/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,19 @@ const config: PartialDeep<Config> = {
kinds: [],
},
},
smtp: {
enabled: true,
connection: {
host: 'localhost',
port: 1025,
secure: false,
auth: undefined,
tls: {
rejectUnauthorized: false,
},
},
from: '"Bailo 📝" <bailo@example.org>',
},
log: {
level: 'debug',
},
Expand Down Expand Up @@ -79,6 +92,18 @@ const config: PartialDeep<Config> = {
userIdAttribute: '',
},
},
avScanning: {
clamdscan: {
host: '127.0.0.1',
port: 8080,
},

modelscan: {
protocol: 'http',
host: '127.0.0.1',
port: 8081,
},
},
mongo: {
uri: 'mongodb://localhost:27017/bailo?directConnection=true',
user: undefined,
Expand Down
42 changes: 4 additions & 38 deletions backend/test/services/__snapshots__/release.spec.ts.snap
Original file line number Diff line number Diff line change
@@ -1,43 +1,9 @@
// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html

exports[`services > release > getModelReleases > good 1`] = `
[
{
"modelId": "modelId",
},
]
`;
exports[`services > release > getModelReleases > good 1`] = `undefined`;

exports[`services > release > getModelReleases > good 2`] = `
[
{
"updatedAt": -1,
},
]
`;
exports[`services > release > getModelReleases > good 2`] = `undefined`;

exports[`services > release > getModelReleases > good 3`] = `
[
{
"as": "model",
"foreignField": "id",
"from": "v2_models",
"localField": "modelId",
},
]
`;
exports[`services > release > getModelReleases > good 3`] = `undefined`;

exports[`services > release > getModelReleases > good 4`] = `
[
{
"$set": {
"model": {
"$arrayElemAt": [
"$model",
0,
],
},
},
},
]
`;
exports[`services > release > getModelReleases > good 4`] = `undefined`;
Loading