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.

Sign up for GitHub

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

refactor: Use JS regular expression syntax, not path-to-regexp, in RegexRouter #3170

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor: Use JS regular expression syntax, not path-to-regexp, in RegexRouter #3170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
187 changes: 100 additions & 87 deletions companion/lib/Service/OscApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { CoreBase } from '../Core/Base.js'
import { parseColorToNumber, rgb } from '../Resources/Util.js'
import { formatLocation } from '@companion-app/shared/ControlId.js'
import { RegexRouter } from './RegexRouter.js'
import { Bank, Location, Page, VariableName } from './RoutePatterns.js'
import type { Registry } from '../Registry.js'
import type { OscReceivedMessage } from 'osc'
import type { ControlLocation } from '@companion-app/shared/Model/Common.js'
Expand Down Expand Up @@ -50,94 +51,106 @@ export class ServiceOscApi extends CoreBase {
}

#setupLegacyOscRoutes(): void {
this.#router.addPath('/press/bank/:page/:bank', (match: Record<string, string>, message: OscReceivedMessage) => {
if (!this.#isLegacyRouteAllowed()) return

const controlId = this.page.getControlIdAtOldBankIndex(Number(match.page), Number(match.bank))
if (!controlId) return

if (message.args.length > 0 && message.args[0].type == 'i' && message.args[0].value == 1) {
this.logger.info(`Got /press/bank/ (press) for ${controlId}`)
this.controls.pressControl(controlId, true, undefined)
} else if (message.args.length > 0 && message.args[0].type == 'i' && message.args[0].value == 0) {
this.logger.info(`Got /press/bank/ (release) for ${controlId}`)
this.controls.pressControl(controlId, false, undefined)
} else {
this.logger.info(`Got /press/bank/ (trigger)${controlId}`)
this.controls.pressControl(controlId, true, undefined)

setTimeout(() => {
this.logger.info(`Auto releasing /press/bank/ (trigger)${controlId}`)
this.#router.addRoute(
`/press/bank/${Page}/${Bank}`,
(match: Record<string, string>, message: OscReceivedMessage) => {
if (!this.#isLegacyRouteAllowed()) return

const controlId = this.page.getControlIdAtOldBankIndex(Number(match.page), Number(match.bank))
if (!controlId) return

if (message.args.length > 0 && message.args[0].type == 'i' && message.args[0].value == 1) {
this.logger.info(`Got /press/bank/ (press) for ${controlId}`)
this.controls.pressControl(controlId, true, undefined)
} else if (message.args.length > 0 && message.args[0].type == 'i' && message.args[0].value == 0) {
this.logger.info(`Got /press/bank/ (release) for ${controlId}`)
this.controls.pressControl(controlId, false, undefined)
}, 20)
} else {
this.logger.info(`Got /press/bank/ (trigger)${controlId}`)
this.controls.pressControl(controlId, true, undefined)

setTimeout(() => {
this.logger.info(`Auto releasing /press/bank/ (trigger)${controlId}`)
this.controls.pressControl(controlId, false, undefined)
}, 20)
}
}
})

this.#router.addPath('/style/bgcolor/:page/:bank', (match: Record<string, string>, message: OscReceivedMessage) => {
if (!this.#isLegacyRouteAllowed()) return

if (message.args.length > 2) {
const r = message.args[0].value
const g = message.args[1].value
const b = message.args[2].value
if (typeof r === 'number' && typeof g === 'number' && typeof b === 'number') {
const controlId = this.page.getControlIdAtOldBankIndex(Number(match.page), Number(match.bank))
if (!controlId) return

const control = this.controls.getControl(controlId)
if (control && control.supportsStyle) {
this.logger.info(`Got /style/bgcolor for ${controlId}`)
control.styleSetFields({ bgcolor: rgb(r, g, b) })
} else {
this.logger.info(`Got /style/bgcolor for unknown control: ${controlId}`)
)

this.#router.addRoute(
`/style/bgcolor/${Page}/${Bank}`,
(match: Record<string, string>, message: OscReceivedMessage) => {
if (!this.#isLegacyRouteAllowed()) return

if (message.args.length > 2) {
const r = message.args[0].value
const g = message.args[1].value
const b = message.args[2].value
if (typeof r === 'number' && typeof g === 'number' && typeof b === 'number') {
const controlId = this.page.getControlIdAtOldBankIndex(Number(match.page), Number(match.bank))
if (!controlId) return

const control = this.controls.getControl(controlId)
if (control && control.supportsStyle) {
this.logger.info(`Got /style/bgcolor for ${controlId}`)
control.styleSetFields({ bgcolor: rgb(r, g, b) })
} else {
this.logger.info(`Got /style/bgcolor for unknown control: ${controlId}`)
}
}
}
}
})

this.#router.addPath('/style/color/:page/:bank', (match: Record<string, string>, message: OscReceivedMessage) => {
if (!this.#isLegacyRouteAllowed()) return

if (message.args.length > 2) {
const r = message.args[0].value
const g = message.args[1].value
const b = message.args[2].value
if (typeof r === 'number' && typeof g === 'number' && typeof b === 'number') {
const controlId = this.page.getControlIdAtOldBankIndex(Number(match.page), Number(match.bank))
if (!controlId) return

const control = this.controls.getControl(controlId)
if (control && control.supportsStyle) {
this.logger.info(`Got /style/color for ${controlId}`)
control.styleSetFields({ color: rgb(r, g, b) })
} else {
this.logger.info(`Got /style/color for unknown control: ${controlId}`)
)

this.#router.addRoute(
`/style/color/${Page}/${Bank}`,
(match: Record<string, string>, message: OscReceivedMessage) => {
if (!this.#isLegacyRouteAllowed()) return

if (message.args.length > 2) {
const r = message.args[0].value
const g = message.args[1].value
const b = message.args[2].value
if (typeof r === 'number' && typeof g === 'number' && typeof b === 'number') {
const controlId = this.page.getControlIdAtOldBankIndex(Number(match.page), Number(match.bank))
if (!controlId) return

const control = this.controls.getControl(controlId)
if (control && control.supportsStyle) {
this.logger.info(`Got /style/color for ${controlId}`)
control.styleSetFields({ color: rgb(r, g, b) })
} else {
this.logger.info(`Got /style/color for unknown control: ${controlId}`)
}
}
}
}
})

this.#router.addPath('/style/text/:page/:bank', (match: Record<string, string>, message: OscReceivedMessage) => {
if (!this.#isLegacyRouteAllowed()) return

if (message.args.length > 0) {
const text = message.args[0].value
if (typeof text === 'string') {
const controlId = this.page.getControlIdAtOldBankIndex(Number(match.page), Number(match.bank))
if (!controlId) return

const control = this.controls.getControl(controlId)
if (control && control.supportsStyle) {
this.logger.info(`Got /style/text for ${controlId}`)
control.styleSetFields({ text: text })
} else {
this.logger.info(`Got /style/color for unknown control: ${controlId}`)
)

this.#router.addRoute(
`/style/text/${Page}/${Bank}`,
(match: Record<string, string>, message: OscReceivedMessage) => {
if (!this.#isLegacyRouteAllowed()) return

if (message.args.length > 0) {
const text = message.args[0].value
if (typeof text === 'string') {
const controlId = this.page.getControlIdAtOldBankIndex(Number(match.page), Number(match.bank))
if (!controlId) return

const control = this.controls.getControl(controlId)
if (control && control.supportsStyle) {
this.logger.info(`Got /style/text for ${controlId}`)
control.styleSetFields({ text: text })
} else {
this.logger.info(`Got /style/color for unknown control: ${controlId}`)
}
}
}
}
})
)

this.#router.addPath('/rescan', (_match: Record<string, string>, _message: OscReceivedMessage) => {
this.#router.addRoute('/rescan', (_match: Record<string, string>, _message: OscReceivedMessage) => {
if (!this.#isLegacyRouteAllowed()) return

this.logger.info('Got /rescan 1')
Expand All @@ -149,22 +162,22 @@ export class ServiceOscApi extends CoreBase {

#setupNewOscRoutes() {
// controls by location
this.#router.addPath('/location/:page/:row/:column/press', this.#locationPress)
this.#router.addPath('/location/:page/:row/:column/down', this.#locationDown)
this.#router.addPath('/location/:page/:row/:column/up', this.#locationUp)
this.#router.addPath('/location/:page/:row/:column/rotate-left', this.#locationRotateLeft)
this.#router.addPath('/location/:page/:row/:column/rotate-right', this.#locationRotateRight)
this.#router.addPath('/location/:page/:row/:column/step', this.#locationStep)
this.#router.addRoute(`/location/${Location}/press`, this.#locationPress)
this.#router.addRoute(`/location/${Location}/down`, this.#locationDown)
this.#router.addRoute(`/location/${Location}/up`, this.#locationUp)
this.#router.addRoute(`/location/${Location}/rotate-left`, this.#locationRotateLeft)
this.#router.addRoute(`/location/${Location}/rotate-right`, this.#locationRotateRight)
this.#router.addRoute(`/location/${Location}/step`, this.#locationStep)

this.#router.addPath('/location/:page/:row/:column/style/text', this.#locationSetStyleText)
this.#router.addPath('/location/:page/:row/:column/style/color', this.#locationSetStyleColor)
this.#router.addPath('/location/:page/:row/:column/style/bgcolor', this.#locationSetStyleBgcolor)
this.#router.addRoute(`/location/${Location}/style/text`, this.#locationSetStyleText)
this.#router.addRoute(`/location/${Location}/style/color`, this.#locationSetStyleColor)
this.#router.addRoute(`/location/${Location}/style/bgcolor`, this.#locationSetStyleBgcolor)

// custom variables
this.#router.addPath('/custom-variable/:name/value', this.#customVariableSetValue)
this.#router.addRoute(`/custom-variable/${VariableName}/value`, this.#customVariableSetValue)

// surfaces
this.#router.addPath('/surfaces/rescan', this.#surfacesRescan)
this.#router.addRoute('/surfaces/rescan', this.#surfacesRescan)
}

/**
Expand Down
25 changes: 4 additions & 21 deletions companion/lib/Service/RegexRouter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import { pathToRegexp } from 'path-to-regexp'

/**
* A regex based route
*/
Expand Down Expand Up @@ -51,25 +49,10 @@ export class RegexRouter {
/**
* Add a route to the router
*/
addRegex(regexp: RegExp, handler: RouteHandler): void {
if (!regexp || !handler) throw new Error('Invalid route parameters')
this.#routes.push({ regexp, handler })
}

/**
* Add a route to the router, using the express style path syntax
*/
addPath(path: string, handler: PathRouteHandler): void {
const { regexp, keys } = pathToRegexp(path)
Copy link
Member

@Julusian Julusian Dec 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm wondering whether we should be keeping pathToRegexp and this method to use for the osc api. Because that api is using typical slash separated components, so wouldn't be fighting the library?

The reason I started using pathToRegexp was to make the regexes simple and resilient with minimal effort (the library will have much better redos and similar protections than we will think to write)

I don't know though, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My taste -- I emphasize this is a taste as this is your turf 🙂 -- is to prefer built-in language function over libraries, when they do similar things. Regular expression syntax is in the JS author's toolkit. A new regular expression language is not. (And theoretically could have its own corner cases.)

I also prefer there be fewer ways to do it, not more. Two distinct pattern mechanisms seems worse than one -- perhaps than either one.

Regular expression pathological cases happen when you have repetition with possibility of matching multiple ways. None of these cases have that. ReDOS simply isn't a worry here.

Honestly, tho, sequentially testing a list of regular expressions isn't the right way for either of these cases compared to actual parsing. For OSC I'd address.split('/') and iterate through the components. For TCP I'd incrementally tokenize the string. I'd use some switches to invoke the intended operation -- and return informative errors for contextual errors.

But I'd never start writing that patch if I didn't know it'd be accepted. 🙂 Regular expressions aren't right for this, but for a fixed list of a couple dozen syntaxes they're bearable, in a patch with the least code most likely to be accepted.

Copy link
Member

Choose a reason for hiding this comment

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

A new regular expression language is not. (And theoretically could have its own corner cases.)
Honestly, tho, sequentially testing a list of regular expressions isn't the right way for either of these cases compared to actual parsing.

True, but I found pathToRegexp because I was curious how express (or koa?) did its route matching. its a pretty widely used and known syntax because of both of those.
Which I think is where I started on this; wanting to setup the osc api in a way that is familiar to adding routes to express.

I think that this regex implementation is much more easily readable than tokenizing would be. Thats the nice thing with this pathToRegexp implementation is that with just a little knowledge on the syntax of the paths, it is easy to skim read each rule being checked and find what you are looking for.

I think this is slightly lost in this PR because it hides some things (format of location), but it makes it more readable in other ways as it highlights which bits of the string are matching something while keeping the exact syntax out of it.

So I am tempted by this, just not convinced by tokenizing


this.addRegex(regexp, (match, ...args) => {
const values: Record<string, string> = {}
for (let i = 0; i < keys.length; i++) {
const key = keys[i]
values[key.name] = values[key.name] ?? match[i + 1]
}

return handler(values, ...args)
addRoute(route: string, handler: PathRouteHandler): void {
this.#routes.push({
regexp: new RegExp(`^${route}$`, 'i'),
handler: (match, ...args) => handler(match.groups ?? {}, ...args),
})
}
}
11 changes: 11 additions & 0 deletions companion/lib/Service/RoutePatterns.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Historic parsing behavior mapped variables to non-'/'-delimiter characters.
// Continue to do so even if matching something more precise would be more
// sensible.
export const Element = '[^/]+?'

export const Page = `(?<page>${Element})`
export const Location = `${Page}/(?<row>${Element})/(?<column>${Element})`

export const Bank = `(?<bank>${Element})`

export const VariableName = `(?<name>${Element})`
Loading
Loading