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

IR96334 · 2024-10-23T08:56:32Z

WIP PR for advanced semver range queries as stated in https://docs.npmjs.com/cli/v6/using-npm/semver#caret-ranges-123-025-004

…r value

…ge query

…ion script

backend/src/services/release.ts

JR40159 · 2024-11-13T08:35:56Z

backend/src/services/release.ts

+        {
+          $or: [
+            {
+              'semver.major': { $lte: upperSemverObj.major },


If upperSemverObj doesn't exist, this part of both queries will break

Checks added for upperSemver, etc

JR40159 · 2024-11-13T09:17:17Z

backend/src/services/release.ts

+    upperSemverObj = semverStringToObject(upperSemverTrimmed)
+  }
+
+  if (inclusivity)


This function is a lot simpler and more readable, good job!
To improve upon this a bit more I would move all the logic needed to convert the semverRangeStandardised content into a form that can be used to generate this mongo query into a single function. At the moment this function contains multiple variable variables and function calls to convert the various semver formats when all you really need at this point is just the lowerSemverObj and upperSemverObj. Also your exclusivity logic doesn't currently work as it doesn't allow for half the semver query being inclusive.

Once you've got your semverRangeStandardised and checked it's not null you could capture all the logic up to this point of the mongo query in a single function, something like:

function parseSemverQuery(semverRangeStandardised: string): { upper: { major: string; minor: string; patch: string; inclusive: boolean } lower: { major: string; minor: string; patch: string; inclusive: boolean } } { //parse semverRangeStandardised }

Then to reduce duplication in the inclusivity logic I would use if statements to set either $gte or $gt and $lse or $ls rather than repeat all the other bits of the mongo query in all the conditions. Eg:

if (semverQuery.upper.inclusive) { lowerComparator = '$gte' } else { lowerComparator = '$gt' } if (semverQuery.lower.inclusive) { uppercomparator = '$lse' } else { uppercomparator = '$ls' }

Then you should be able to use this two variable in the relevant places in your single mongo query object Eg 'semver.major': { [lowerComparator]: semverQuery.lower.major }

Changes to semverRangeStandardised added, also added dynamic query stuff for inclusivity.

backend/src/services/release.ts

JR40159 · 2024-11-25T10:57:09Z

backend/src/services/release.ts

+    upperSemver: string = '',
+    lowerInclusivity = false,
+    upperInclusivity = false
+  if (expressionA.includes('>')) {


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, '')

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

backend/src/services/release.ts

…mver-for-releases

JR40159 · 2024-12-02T11:12:28Z

backend/src/services/release.ts

+  )[]
+}
+
+export function getQuerySyntax(querySemver: string, modelID: string) {


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()

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

JR40159 · 2024-12-02T11:14:06Z

backend/src/services/release.ts

@@ -295,6 +335,142 @@ export async function getReleaseBySemver(user: UserInterface, modelId: string, s
  return release
 }

+function getSemverQueryBounds(querySemver: string) {


Could we change this to something like parseSemverQuery()?

JR40159 · 2024-12-02T11:18:12Z

backend/src/services/release.ts

+  const semverRangeStandardised = semver.validRange(querySemver, { includePrerelease: false })
+
+  if (!semverRangeStandardised) {
+    throw BadReq(`Semver range, '${querySemver}' is invalid.`)


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})

JR40159 · 2024-12-02T11:23:25Z

backend/src/services/release.ts

+  //If lower semver
+  if (expressionA.includes('>')) {
+    lowerInclusivity = expressionA.includes('>=')
+    lowerSemver = expressionA.replace(/[<>=]/g, '')


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

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

JR40159 · 2024-12-02T11:31:57Z

backend/test/services/release.spec.ts

+    async () => {
+      const testQuery = getQuerySyntax('1.0.x', 'testModelID')
+      expect(testQuery).toBe({
+        modelId: 'testModelID',


For large expects like this one, I would use snapshots.

JR40159 · 2024-12-02T11:33:09Z

backend/test/services/release.spec.ts


    expect(releaseModelMocks.match.mock.calls.at(0)).toMatchSnapshot()
    expect(releaseModelMocks.sort.mock.calls.at(0)).toMatchSnapshot()
    expect(releaseModelMocks.lookup.mock.calls.at(0)).toMatchSnapshot()
    expect(releaseModelMocks.append.mock.calls.at(0)).toMatchSnapshot()
  })

+  //test good - give good semver range with lower and upper bounds, returns valid query
+  //test fail - invalid range
+  test('getQuerySyntax > good'),


I think getQuerySyntax shouldn't be exported so here you would need to test calling getModelReleases with a semverQuery and verify that the match is what you expect.

JR40159 · 2024-12-02T12:02:59Z

backend/test/services/release.spec.ts


    expect(releaseModelMocks.match.mock.calls.at(0)).toMatchSnapshot()
    expect(releaseModelMocks.sort.mock.calls.at(0)).toMatchSnapshot()
    expect(releaseModelMocks.lookup.mock.calls.at(0)).toMatchSnapshot()
    expect(releaseModelMocks.append.mock.calls.at(0)).toMatchSnapshot()
  })

+  //test good - give good semver range with lower and upper bounds, returns valid query
+  //test fail - invalid range
+  test('getQuerySyntax > good'),


I don't think your new tests are correctly running. The whole test including the async functions needs to be included in the call to test(). As such this line should be test('getQuerySyntax > good', and 432 should have the closing bracket.

Make sure you look at the test coverage to check if there's anywhere you've missed.

ARADDCC012 · 2024-12-03T14:05:35Z

backend/src/services/release.ts

@@ -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>)


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

Hmmm... fixed

ARADDCC012 · 2024-12-03T14:08:05Z

backend/src/services/release.ts

+    upperSemver = expressionB.replace(/[<=]/g, '')
+  }
+
+  let lowerSemverObj, upperSemverObj


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

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

ARADDCC012 · 2024-12-03T14:14:54Z

backend/src/services/release.ts

-    upperSemver: string = '',
-    lowerInclusivity = false,
-    upperInclusivity = false
+  let lowerInclusivity, lowerSemver, upperInclusivity, upperSemver


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

ARADDCC012 · 2024-12-03T14:22:50Z

backend/src/services/release.ts

+  if (!semver) {
+    return ''
+  }
+  let metadata: string


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

IR96334 added 13 commits September 24, 2024 13:42

Can now receive one release by string comparison in mongoose query

3ad2e86

Added basic semver range check, getters and setters for release semve…

1c421d7

…r value

Add series of queries for different types

9e10ab3

Added @types/semver, small modifications to range query

ae35fc8

Now plugs into releases endpoint instead of singular release.

82bb578

Changed package name

2d8b532

Simplified and grouped functionality. Also added primitive hyphen ran…

a7b05b8

…ge query

Fixed caret query

725eb87

Added comments for each range query type for explanation

da9110c

Added migrations script and updated schema getter to allow for migrat…

6df897e

…ion script

Refactor

d050140

Bugfixed refactor, removed old code

1e56d7b

merge main branch

ce0f95e

github-advanced-security bot found potential problems Nov 12, 2024

View reviewed changes

backend/src/services/release.ts Fixed Show resolved Hide resolved

JR40159 requested changes Nov 13, 2024

View reviewed changes

change as per PR

8e7f254

github-advanced-security bot found potential problems Nov 18, 2024

View reviewed changes

backend/src/services/release.ts Fixed Show fixed Hide fixed

Simplified code and added functionality for more queries

235f1f3

github-advanced-security bot found potential problems Nov 19, 2024

View reviewed changes

backend/src/services/release.ts Fixed Show fixed Hide fixed

backend/src/services/release.ts Fixed Show fixed Hide fixed

backend/src/services/release.ts Fixed Show fixed Hide fixed

Simplified queries massively, now no longer overwrites previous query

47106bd

github-advanced-security bot found potential problems Nov 21, 2024

View reviewed changes

backend/src/services/release.ts Fixed Show fixed Hide fixed

backend/src/services/release.ts Fixed Show fixed Hide fixed

backend/src/services/release.ts Fixed Show fixed Hide fixed

IR96334 added 2 commits November 22, 2024 14:54

Simplified queries further, added queryboundinterface and queue

ffc7b03

Removed unnecessary comments

0215e0e

IR96334 requested a review from JR40159 November 22, 2024 14:57

github-advanced-security bot found potential problems Nov 22, 2024

View reviewed changes

backend/src/services/release.ts Fixed Show fixed Hide fixed

backend/src/services/release.ts Fixed Show fixed Hide fixed

JR40159 requested changes Nov 25, 2024

View reviewed changes

ARADDCC012 reviewed Nov 25, 2024

View reviewed changes

backend/src/services/release.ts Outdated Show resolved Hide resolved

ARADDCC012 reviewed Nov 25, 2024

View reviewed changes

backend/src/services/release.ts Outdated Show resolved Hide resolved

ARADDCC012 reviewed Nov 25, 2024

View reviewed changes

backend/src/services/release.ts Outdated Show resolved Hide resolved

ARADDCC012 reviewed Nov 25, 2024

View reviewed changes

backend/src/services/release.ts Outdated Show resolved Hide resolved

IR96334 and others added 12 commits November 28, 2024 12:30

Small test change

b5658be

Test changes

f2ff99f

Trying fix for python tests

9604ff1

Fixed smtp tests

f532454

Merge branch 'main' into BAI-1111-allow-semver-matching-and-latest-se…

223dbad

…mver-for-releases

Fix for smtp error

fcf9a0a

Changed host to local

0bc56a7

Added mock avScanning config

84dfdac

Updated smtp unit test mock config

9f6eb68

Updated avScanning mock config ports

d15fe16

Moved smtp test config into global mock config

0546f95

Changed name of migration script

5118315

IR96334 marked this pull request as ready for review December 2, 2024 10:09

JR40159 requested changes Dec 2, 2024

View reviewed changes

ARADDCC012 reviewed Dec 3, 2024

View reviewed changes

IR96334 added 2 commits December 3, 2024 14:45

Changes to testing

ae80725

Added more changes as per PR

8f31592

IR96334 requested review from ARADDCC012 and JR40159 December 3, 2024 17:23

IR96334 added 3 commits December 3, 2024 17:25

Removed error

f8ca356

Fixed tests, mocking, added tests, and adjsuted getModelReleases

d6f486f

Fixed release query functionality

8d0cb76

JR40159 approved these changes Dec 6, 2024

View reviewed changes

IR96334 merged commit 7f494c3 into main Dec 6, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

IR96334 commented Oct 23, 2024

JR40159 Nov 13, 2024

IR96334 Nov 21, 2024

JR40159 Nov 13, 2024 •

edited

Loading

IR96334 Nov 21, 2024

JR40159 Nov 25, 2024

IR96334 Nov 25, 2024

JR40159 Dec 2, 2024

IR96334 Dec 2, 2024

JR40159 Dec 2, 2024

IR96334 Dec 2, 2024

JR40159 Dec 2, 2024

IR96334 Dec 2, 2024

JR40159 Dec 2, 2024 •

edited

Loading

IR96334 Dec 2, 2024

JR40159 Dec 2, 2024

IR96334 Dec 4, 2024

JR40159 Dec 2, 2024

IR96334 Dec 4, 2024

JR40159 Dec 2, 2024

JR40159 Dec 2, 2024

IR96334 Dec 4, 2024

ARADDCC012 Dec 3, 2024

IR96334 Dec 3, 2024

ARADDCC012 Dec 3, 2024 •

edited

Loading

IR96334 Dec 3, 2024

ARADDCC012 Dec 3, 2024 •

edited

Loading

IR96334 Dec 3, 2024

ARADDCC012 Dec 3, 2024

IR96334 Dec 3, 2024

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

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

Conversation

IR96334 commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JR40159 Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JR40159 Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ARADDCC012 Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ARADDCC012 Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JR40159 Nov 13, 2024 •

edited

Loading

JR40159 Dec 2, 2024 •

edited

Loading

ARADDCC012 Dec 3, 2024 •

edited

Loading

ARADDCC012 Dec 3, 2024 •

edited

Loading