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 18 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: 8 additions & 0 deletions backend/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions backend/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@
"@types/mjml": "^4.7.4",
"@types/mongoose-delete": "^1.0.6",
"@types/morgan": "^1.9.9",
"@types/semver": "^7.5.8",
"@types/node": "^22.7.5",
"@types/nodemailer": "^6.4.16",
"@types/shelljs": "^0.8.15",
Expand Down
16 changes: 16 additions & 0 deletions backend/src/migrations/009_convert_semver_string_to_object.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import Release from '../models/Release.js'

export async function up() {
const releases = await Release.find({})
for (const release of releases) {
const semver = release.get('semver')
if (semver !== undefined && typeof semver === typeof '') {
release.set('semver', semver)
await release.save()
}
}
}

export async function down() {
/* NOOP */
}
27 changes: 25 additions & 2 deletions backend/src/models/Release.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { Document, model, Schema } from 'mongoose'
import MongooseDelete from 'mongoose-delete'

import { semverObjectToString, semverStringToObject } from '../services/release.js'

// This interface stores information about the properties on the base object.
// It should be used for plain object representations, e.g. for sending to the
// client.
Expand Down Expand Up @@ -35,12 +37,31 @@ export interface ImageRef {
// object from Mongoose it should use this interface
export type ReleaseDoc = ReleaseInterface & Document<any, any, ReleaseInterface>

const ReleaseSchema = new Schema<ReleaseInterface>(
export interface SemverObject {
major: number
minor: number
patch: number
metadata?: string
}

const ReleaseSchema = new Schema<ReleaseInterface & { semver: string | SemverObject }>(
{
modelId: { type: String, required: true },
modelCardVersion: { type: Number, required: true },

semver: { type: String, required: true },
semver: {
type: Schema.Types.Mixed,
required: true,
set: function (semver: string) {
return semverStringToObject(semver)
},
get: function (semver: SemverObject | string) {
if (typeof semver === 'string') {
return semver
} else return semverObjectToString(semver)
},
},

notes: { type: String, required: true },

minor: { type: Boolean, required: true },
Expand All @@ -60,6 +81,8 @@ const ReleaseSchema = new Schema<ReleaseInterface>(
{
timestamps: true,
collection: 'v2_releases',
toJSON: { getters: true },
toObject: { getters: true },
},
)

Expand Down
6 changes: 5 additions & 1 deletion backend/src/routes/v2/release/getReleases.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ export const getReleasesSchema = z.object({
required_error: 'Must specify model id as URL parameter',
}),
}),
query: z.object({
q: z.string().optional(),
Copy link
Member

Choose a reason for hiding this comment

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

Could you include an example and description here for the API docs please

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

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

}),
})

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

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

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

import { ReleaseAction } from '../connectors/authorisation/actions.js'
import authorisation from '../connectors/authorisation/index.js'
import { FileInterface } from '../models/File.js'
import { ModelDoc, ModelInterface } from '../models/Model.js'
import Release, { ImageRef, ReleaseDoc, ReleaseInterface } from '../models/Release.js'
import Release, { ImageRef, ReleaseDoc, ReleaseInterface, SemverObject } from '../models/Release.js'
import ResponseModel, { ResponseKind } from '../models/Response.js'
import { UserInterface } from '../models/User.js'
import { WebhookEvent } from '../models/Webhook.js'
Expand Down Expand Up @@ -55,7 +56,8 @@
const fileNames: Array<string> = []

for (const fileId of release.fileIds) {
let file
let file: Document<unknown, any, FileInterface> &
ARADDCC012 marked this conversation as resolved.
Show resolved Hide resolved
Omit<FileInterface & Required<{ _id: Schema.Types.ObjectId }>, never>
try {
file = await getFileById(user, fileId)
} catch (e) {
Expand Down Expand Up @@ -237,9 +239,11 @@
export async function getModelReleases(
user: UserInterface,
modelId: string,
q?: string,
ARADDCC012 marked this conversation as resolved.
Show resolved Hide resolved
): Promise<Array<ReleaseDoc & { model: ModelInterface; files: FileInterface[] }>> {
const query = q === undefined ? { modelId } : getQuerySyntax(q, modelId)
ARADDCC012 marked this conversation as resolved.
Show resolved Hide resolved
const results = await Release.aggregate()
.match({ modelId })
.match(query!)
ARADDCC012 marked this conversation as resolved.
Show resolved Hide resolved
.sort({ updatedAt: -1 })
.lookup({ from: 'v2_models', localField: 'modelId', foreignField: 'id', as: 'model' })
.append({ $set: { model: { $arrayElemAt: ['$model', 0] } } })
Expand All @@ -248,7 +252,9 @@
const model = await getModelById(user, modelId)

const auths = await authorisation.releases(user, model, results, ReleaseAction.View)
return results.filter((_, i) => auths[i].success)
return results
ARADDCC012 marked this conversation as resolved.
Show resolved Hide resolved
.filter((_, i) => auths[i].success)
.map((result) => ({ ...result, semver: semverObjectToString(result.semver) }))

Check failure on line 257 in backend/src/services/release.ts

View workflow job for this annotation

GitHub Actions / unit_testing

test/services/release.spec.ts > services > release > getModelReleases > good

TypeError: results.filter(...).map is not a function ❯ Module.getModelReleases src/services/release.ts:257:6 ❯ test/services/release.spec.ts:345:5
}

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

export function semverStringToObject(semver: string) {
ARADDCC012 marked this conversation as resolved.
Show resolved Hide resolved
const vIdentifierIndex = semver.indexOf('v')
const trimmedSemver = vIdentifierIndex === -1 ? semver : semver.slice(vIdentifierIndex + 1)
const [version, metadata] = trimmedSemver.split('-')
const [major, minor, patch] = version.split('.')
const majorNum: number = Number(major)
const minorNum: number = Number(minor)
const patchNum: number = Number(patch)
return { major: majorNum, minor: minorNum, patch: patchNum, ...(metadata && { metadata }) }
}

export function semverObjectToString(semver: SemverObject) {
if (!semver) {
return ''
}
let metadata: string
Copy link
Member

Choose a reason for hiding this comment

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

This type isn't strictly true as it's undefined until you assign a value. Instead I'd suggest let metadata = ''

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 (semver.metadata != undefined) {
metadata = `-${semver.metadata}`
} else {
metadata = ``
}
return `${semver.major}.${semver.minor}.${semver.patch}${metadata}`
}

export async function getReleaseBySemver(user: UserInterface, modelId: string, semver: string) {
const model = await getModelById(user, modelId)
const semverObj = semverStringToObject(semver)
const release = await Release.findOne({
modelId,
semver,
semver: semverObj,
})

if (!release) {
Expand All @@ -295,6 +326,168 @@
return release
}

function getSemverQueryBounds(querySemver: string) {
Copy link
Member

Choose a reason for hiding this comment

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

Could we change this to something like parseSemverQuery()?

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 semverRangeStandardised = semver.validRange(querySemver, { includePrerelease: false })

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

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

let lowerSemver: string = '',
upperSemver: string = '',
lowerInclusivity = false,
upperInclusivity = false
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('>', '')
Fixed Show fixed Hide fixed
}
//do check for expressionB
if (expressionB) {
if (expressionB.includes('=')) {
upperSemver = expressionB.replace('<=', '')
upperInclusivity = true
} else {
upperSemver = expressionB.replace('<', '')
Fixed Show fixed Hide fixed
Fixed Show fixed Hide fixed
}
}
} else {
if (expressionA.includes('=')) {
upperSemver = expressionA.replace('<=', '')
upperInclusivity = true
} else {
upperSemver = expressionA.replace('<', '')
Fixed Show fixed Hide fixed
Fixed Show fixed Hide fixed
Fixed Show fixed Hide fixed
}
}

let lowerSemverObj: { metadata?: string | undefined; major: number; minor: number; patch: number } = {
ARADDCC012 marked this conversation as resolved.
Show resolved Hide resolved
major: NaN,
minor: NaN,
patch: NaN,
},
upperSemverObj: { metadata?: string | undefined; major: number; minor: number; patch: number } = {
ARADDCC012 marked this conversation as resolved.
Show resolved Hide resolved
major: NaN,
minor: NaN,
patch: NaN,
}
if (lowerSemver) {
lowerSemverObj = semverStringToObject(lowerSemver)
}

if (upperSemver) {
upperSemverObj = semverStringToObject(upperSemver)
}

return { lowerSemverObj, upperSemverObj, lowerInclusivity, upperInclusivity }
}

function getQuerySyntax(querySemver: string | undefined, modelID: string) {
if (querySemver === undefined) {
return {
modelId: modelID,
}
}

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[] = []

if (lowerSemverObj) {
const lowerQuery: QueryBoundInterface = {
$or: [
{
'semver.major': { $gte: lowerSemverObj.major },
'semver.minor': { $gte: lowerSemverObj.minor },
'semver.patch': lowerInclusivity ? { $gte: lowerSemverObj.patch } : { $gt: lowerSemverObj.patch },
},
{
'semver.major': { $gt: lowerSemverObj.major },
},
{
'semver.major': { $gte: lowerSemverObj.major },
'semver.minor': { $gt: lowerSemverObj.minor },
},
],
}
queryQueue.push(lowerQuery)
}

if (upperSemverObj) {
const upperQuery: QueryBoundInterface = {
$or: [
{
'semver.major': { $lte: upperSemverObj.major },
'semver.minor': { $lte: upperSemverObj.minor },
'semver.patch': upperInclusivity ? { $lte: upperSemverObj.patch } : { $lt: upperSemverObj.patch },
},
{
'semver.major': { $lt: upperSemverObj.major },
},
{
'semver.major': { $lte: upperSemverObj.major },
'semver.minor': { $lt: upperSemverObj.minor },
},
],
}
queryQueue.push(upperQuery)
}

const combinedQuery = {
modelId: modelID,
$and: queryQueue,
}

return combinedQuery
}

export async function deleteRelease(user: UserInterface, modelId: string, semver: string) {
const model = await getModelById(user, modelId)
if (model.settings.mirror.sourceModelId) {
Expand Down
Loading
Loading