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

ARADDCC012 · 2024-11-25T11:14:52Z

backend/src/services/release.ts

+    }
+  }
+
+  let lowerSemverObj: { metadata?: string | undefined; major: number; minor: number; patch: number } = {


In a situation where two things are the exact same type, we should avoid redefining types. This allows us to have a single source of truth. If we ever updated that type for any reason, we'd only have to update it in one place and TS would guide us through updating all variables of that type.

In this scenario we should be using the SemverObject type you've already created in backend/src/models/Release.ts for both lowerSemverObj and upperSemverObj.

Removed as was causing bugs and didn't seem strictly necessary.

ARADDCC012 · 2024-11-25T11:17:17Z

backend/src/services/release.ts

+      minor: NaN,
+      patch: NaN,
+    },
+    upperSemverObj: { metadata?: string | undefined; major: number; minor: number; patch: number } = {


Although JS allows us to define multiple variables on one line like you're doing here, I would strongly advise against it as it's simply less readable than defining multiple variables separately.

let lowerSemverObj: SemverObject = { ... } let upperSemverObj: SemverObject = { ... }

Fixed above

ARADDCC012 · 2024-11-25T11:24:24Z

backend/src/services/release.ts

@@ -237,9 +239,11 @@ export async function newReleaseComment(user: UserInterface, modelId: string, se
 export async function getModelReleases(
  user: UserInterface,
  modelId: string,
+  q?: string,


I'm never a fan of single character variable names like this. It's more confusing than simply giving it a proper name.

Consider calling this querySemver as you have later on.

True. Fixed

ARADDCC012 · 2024-11-25T11:25:27Z

backend/src/services/release.ts

 ): Promise<Array<ReleaseDoc & { model: ModelInterface; files: FileInterface[] }>> {
+  const query = q === undefined ? { modelId } : getQuerySyntax(q, modelId)


This can be simplified to const query = getQuerySyntax(q, modelId) as you've already covered off the case of q === undefined inside of getQuerySyntax

Removed the check in getquerysyntax, dealing with it earlier makes more sense to me, but willing to change

ARADDCC012 · 2024-11-25T11:28:15Z

backend/src/services/release.ts

  const results = await Release.aggregate()
-    .match({ modelId })
+    .match(query!)


By using ! we're telling TS that this can't possibly be undefined/null. This is very rarely the correct way to manage types and is likely indicative of a flaw in your getQuerySyntax() logic.

I'd suggest removing this assertion and seeing where undefined/null are possibilities in earlier logic. You should fix it there instead of asserting here.

Turns out I didn't even need it anymore so just removed

JR40159 · 2024-11-25T11:32:52Z

backend/src/routes/v2/release/getReleases.ts

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


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

ARADDCC012 · 2024-11-25T11:33:29Z

backend/src/services/release.ts

@@ -248,7 +252,9 @@ 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)
+  return results


If we care about performance here, we can combine this filter and map into a single reduce:

return results.reduce<(ReleaseInterface & { model: ModelInterface })[]>((updatedResults, result, index) => { if (auths[index].success) { updatedResults.push({ ...result, semver: semverObjectToString(result.semver) }) } return updatedResults }, [])

Added, but wasn't sure on how to implement the updatedResults as I can't push to it as its not an array, etc

ARADDCC012 · 2024-11-25T11:34:44Z

backend/src/services/release.ts

@@ -276,11 +282,36 @@ export async function getReleasesForExport(user: UserInterface, modelId: string,
  return releases
 }

+export function semverStringToObject(semver: string) {


We can give this an explicit return type to ensure it's not broken by accident:

export function semverStringToObject(semver: string): SemverObject { ...

You could do the same for semverObjectToString, but that's less of a concern as it only has to return a string instead of an object of a defined shape.

ARADDCC012 · 2024-11-25T11:51:31Z

backend/src/services/release.ts

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

    for (const fileId of release.fileIds) {
-      let file
+      let file: Document<unknown, any, FileInterface> &


This can be simplified to let file: FileInterfaceDoc | undefined

Don't know why its saying I've done this, but I have changed anyway!

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

JR40159 requested changes Nov 25, 2024

View reviewed changes

ARADDCC012 reviewed Nov 25, 2024

View reviewed changes

IR96334 requested review from ARADDCC012 and JR40159 November 26, 2024 12:54

IR96334 added 5 commits November 28, 2024 11:52

PR changes and added tests

3915659

Small test change

b5658be

Test changes

f2ff99f

Trying fix for python tests

9604ff1

Fixed smtp tests

f532454

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

ARADDCC012 Nov 25, 2024

IR96334 Nov 25, 2024

ARADDCC012 Nov 25, 2024

IR96334 Nov 25, 2024

ARADDCC012 Nov 25, 2024 •

edited

Loading

IR96334 Nov 25, 2024

ARADDCC012 Nov 25, 2024

IR96334 Nov 25, 2024

ARADDCC012 Nov 25, 2024

IR96334 Nov 25, 2024

JR40159 Nov 25, 2024

IR96334 Nov 26, 2024

IR96334 Nov 26, 2024

ARADDCC012 Nov 25, 2024 •

edited

Loading

IR96334 Nov 26, 2024

ARADDCC012 Nov 25, 2024 •

edited

Loading

IR96334 Nov 26, 2024

ARADDCC012 Nov 25, 2024

IR96334 Nov 26, 2024

		): Promise<Array<ReleaseDoc & { model: ModelInterface; files: FileInterface[] }>> {
		const query = q === undefined ? { modelId } : getQuerySyntax(q, modelId)

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

Are you sure you want to change the base?

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

ARADDCC012 Nov 25, 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

ARADDCC012 Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ARADDCC012 Nov 25, 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

ARADDCC012 Nov 25, 2024 •

edited

Loading

ARADDCC012 Nov 25, 2024 •

edited

Loading

ARADDCC012 Nov 25, 2024 •

edited

Loading