SOF-1670 Expose Endpoint To Reload Rundown #88

RasmusAlbrektsen · 2023-10-30T13:05:48Z

This PR uses Koa to create a standalone endpoint that can be used to reload the iNews data for a given rundown, without going through Core. This is simplified by retrieving only a rundownName (the iNews path) rather than using a new property in sofie-server for this purpose. This did, however, require a suffix (_1) to be used with the current implementation of the gateway.

LindvedKrvang

Nice and simple :)

LindvedKrvang · 2023-10-31T06:54:21Z

src/connector.ts

+			if (!this.iNewsFTPHandler.iNewsWatcher) {
+				return
+			}
+			this.iNewsFTPHandler.iNewsWatcher?.ResyncRundown(ctx.params.rundownName + RUNDOWN_EXTERNAL_ID_SUFFIX)


Do you need the ?? You just checked that you had an iNewsWatcher on line 149.
If you still need it, then you shouldn't need the if-statement.

A mistake on my part, i must have forgot to remove it after i made the guard for that very purpose :-)

I have changed it!

LindvedKrvang · 2023-10-31T06:54:41Z

src/connector.ts

+
+		this.koaApp.use(this.koaRouter.routes()).use(this.koaRouter.allowedMethods())
+
+		this.koaApp.listen(3007, () => {})


Would be nice if the port was also in a constant.

Moved into a constant KOA_PORT.

KvelaGorrrrnio · 2023-10-31T07:38:35Z

src/connector.ts

+		const KOA_PORT: number = 3007
+		const RUNDOWN_EXTERNAL_ID_SUFFIX: string = '_1'
+
+		this.koaRouter.get('/reloadData/:rundownName', async (ctx, next): Promise<void> => {


I guess ctx stands for context?

Do we have types for ctx and next?

What is the response, when calling this endpoint? And is it formatted such that the caller can, in a structured way, decipher it?

True, it is short for context.

We do have types, which is e.g. next: Koa.next and that would be nice to add but i decided not to since the type for context: ctx: Koa.ParameterizedContext<any, KoaRouter.IRouterParamContext<any, {}>, any>.

I guess there really shouldn't be a specific response other than maybe success or "failure".

You could use a custom type that encapsulates the data that you need from the koa context.

KvelaGorrrrnio · 2023-10-31T07:43:11Z

src/connector.ts

+		const KOA_PORT: number = 3007
+		const RUNDOWN_EXTERNAL_ID_SUFFIX: string = '_1'
+
+		this.koaRouter.get('/reloadData/:rundownName', async (ctx, next): Promise<void> => {


Perhaps another method than GET should be used, since we are not reading the data out, but triggering a computation.

I did think about this, what is more correct in this scenario? POST and PUT, for instance, would signify that the we actually submit something rather than trigger a computation, right?

For now it has been changed to a POST.

KvelaGorrrrnio · 2023-10-31T07:44:42Z

src/connector.ts

+			if (!this.iNewsFTPHandler.iNewsWatcher) {
+				return
+			}


In the case where we can't reload the data, wouldn't the caller be interested in knowing that the rundown has not been reloaded?

I have added more handling towards this purpose!

…is succesfull in case of wrong rundown name

LindvedKrvang · 2023-11-01T06:44:40Z

src/connector.ts

+		const KOA_PORT: number = 3007
+		const RUNDOWN_EXTERNAL_ID_SUFFIX: string = '_1'
+
+		this.koaRouter.post('/reloadData/:rundownName', async (context, next): Promise<void> => {


This is not following REST standards (maybe that is fine).
If we want this URL to follow REST standards, it should be changed to do so.

Personally, I would prefer it, but I can see arguments for not doing it.

I see your point, and on Sofie Server i'd say it makes a lot of sense. Here, however, i might counter argue that because of the setup where we both actually work on playlists and rundowns to do this operation, it's difficult to just use e.g. rundowns/reloadData/:rundownName since it is used for both playlists and rundowns.

On another note, this is tied to our implementation of Sofie Server alone, so i will name it to fit the standard it follows!

LindvedKrvang · 2023-11-01T06:46:24Z

src/connector.ts

-			await next()
+			try {
+				context.response.body = `Attempting to reload rundown with name ${rundownName}`
+				await this.iNewsFTPHandler.iNewsWatcher.ResyncRundown( rundownName + RUNDOWN_EXTERNAL_ID_SUFFIX)


Are you running lint?
A lot of your PRs (not just this one) has small syntax 'issues'. Here it is an extra space before rundownName.
In your PR for SofieServer for this it appears random whether your imports have spaces or not.

I've never had this issue before with linting, i will be sure to check and do it.

KvelaGorrrrnio

Requesting changes due to two approvals :D

KvelaGorrrrnio · 2023-11-02T07:10:10Z

src/connector.ts

+		const RUNDOWN_EXTERNAL_ID_SUFFIX: string = '_1'
+
+		this.koaRouter.post(
+			'/reloadData/rundowns/:rundownName',


Instead of defining the resource on the action, the action should be defined on the resource as /rundowns/:rundownName/reload-data.

I have changed reloadData to reload-data to avoid clash between case-sensitive and case-insensitive systems (https://blog.restcase.com/5-basic-rest-api-design-guidelines/, see spinal-case).

Thanks for the read, that helped!
I changed it to fit the suggestion, although it is reingest-data instead to follow the instructions provided for the frontend.

KvelaGorrrrnio · 2023-11-02T07:18:39Z

src/connector.ts

+		const KOA_PORT: number = 3007
+		const RUNDOWN_EXTERNAL_ID_SUFFIX: string = '_1'
+
+		this.koaRouter.get('/reloadData/:rundownName', async (ctx, next): Promise<void> => {


You could use a custom type that encapsulates the data that you need from the koa context.

KvelaGorrrrnio · 2023-11-02T07:21:20Z

src/connector.ts

+				}
+
+				try {
+					context.response.body = `Attempting to reload rundown with name ${rundownName}`


The state that is send back should represent the end state of the action. This should be set after the resync action is called.

Now correctly does so!

KvelaGorrrrnio · 2023-11-02T07:27:46Z

src/connector.ts

+				try {
+					context.response.body = `Attempting to reload rundown with name ${rundownName}`
+					await this.iNewsFTPHandler.iNewsWatcher.ResyncRundown(rundownName + RUNDOWN_EXTERNAL_ID_SUFFIX)
+					await next()


Shouldn't next be called before the logic of the endpoint? That way, if there is other middleware, e.g. authentication, it is called before applying the business logic.

Next has been removed since you are absolutely right.

feat: adds koa endpoint for accessing the resycing of a rundown

09d1f53

RasmusAlbrektsen requested review from KvelaGorrrrnio, andr9528 and LindvedKrvang October 30, 2023 13:05

andr9528 approved these changes Oct 30, 2023

View reviewed changes

LindvedKrvang approved these changes Oct 31, 2023

View reviewed changes

fix: changes port to a constant

d0a17c9

KvelaGorrrrnio reviewed Oct 31, 2023

View reviewed changes

RasmusAlbrektsen added 2 commits October 31, 2023 09:53

refactor: adds more error handling and changes the GET to a POST

94f95d9

fix: adds message to succesful call

0c4095b

RasmusAlbrektsen requested a review from KvelaGorrrrnio October 31, 2023 08:56

RasmusAlbrektsen added 2 commits October 31, 2023 13:38

fix: remove misleading message since we cannot handle whether resync …

7e1d1da

…is succesfull in case of wrong rundown name

refactor: moves rundown name to a constant and rearranges some code

eb17bc3

LindvedKrvang approved these changes Nov 1, 2023

View reviewed changes

RasmusAlbrektsen added 2 commits November 1, 2023 08:15

lint: lint fix

0b715e6

refactor: changes url to follow REST

052dda7

KvelaGorrrrnio requested changes Nov 2, 2023

View reviewed changes

KvelaGorrrrnio reviewed Nov 2, 2023

View reviewed changes

refactor: Now correctly handles errors and has typed context

07159a0

RasmusAlbrektsen requested a review from KvelaGorrrrnio November 2, 2023 09:21

KvelaGorrrrnio approved these changes Nov 2, 2023

View reviewed changes

refactor: renames the url to be just reingest rather than reingest-data

6d7ed67

RasmusAlbrektsen merged commit a9c49b5 into develop Nov 3, 2023
5 checks passed

RasmusAlbrektsen deleted the SOF-1670/ExposeEndpointToReloadRundown branch November 3, 2023 07:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOF-1670 Expose Endpoint To Reload Rundown #88

SOF-1670 Expose Endpoint To Reload Rundown #88

RasmusAlbrektsen commented Oct 30, 2023

LindvedKrvang left a comment

LindvedKrvang Oct 31, 2023

RasmusAlbrektsen Oct 31, 2023

RasmusAlbrektsen Oct 31, 2023

LindvedKrvang Oct 31, 2023

RasmusAlbrektsen Oct 31, 2023

KvelaGorrrrnio Oct 31, 2023

RasmusAlbrektsen Oct 31, 2023

KvelaGorrrrnio Nov 2, 2023

KvelaGorrrrnio Oct 31, 2023

RasmusAlbrektsen Oct 31, 2023

RasmusAlbrektsen Oct 31, 2023

KvelaGorrrrnio Oct 31, 2023

RasmusAlbrektsen Oct 31, 2023

LindvedKrvang Nov 1, 2023

RasmusAlbrektsen Nov 1, 2023

RasmusAlbrektsen Nov 1, 2023

LindvedKrvang Nov 1, 2023

RasmusAlbrektsen Nov 1, 2023

KvelaGorrrrnio left a comment

KvelaGorrrrnio Nov 2, 2023

RasmusAlbrektsen Nov 2, 2023

KvelaGorrrrnio Nov 2, 2023

KvelaGorrrrnio Nov 2, 2023

RasmusAlbrektsen Nov 2, 2023

KvelaGorrrrnio Nov 2, 2023

RasmusAlbrektsen Nov 2, 2023


		this.koaApp.use(this.koaRouter.routes()).use(this.koaRouter.allowedMethods())

		this.koaApp.listen(3007, () => {})

SOF-1670 Expose Endpoint To Reload Rundown #88

SOF-1670 Expose Endpoint To Reload Rundown #88

Conversation

RasmusAlbrektsen commented Oct 30, 2023

LindvedKrvang left a 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

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

KvelaGorrrrnio left a 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