Repository: taskforcesh/bullmq
Pull request #538 (2021-05-10 03:07:17)

Fix getJobLogs no reversed pagination
Fixes OptimalBits/bull#2031

manast: Thanks for the PR. I guess it comes from this issue https://github.com/OptimalBits/bull/issues/2031
As I wrote, I think the best would be to introduce an "asc" parameter as we have in the other getters where default would be "false" so that we keep backwards compatibility since there are cases were we want to paginate from newest logs to oldest and others when we want to provide streaming.
roggervalf: > Thanks for the PR. I guess it comes from this issue [OptimalBits/bull#2031](https://github.com/OptimalBits/bull/issues/2031)
> As I wrote, I think the best would be to introduce an "asc" parameter as we have in the other getters where default would be "false" so that we keep backwards compatibility since there are cases were we want to paginate from newest logs to oldest and others when we want to provide streaming.

Sure, in last version I refactored it. Thanks
manast: I am re-studying the code since this was written long time ago and I did not remember it anymore. So basically the "asc" parameter is being used  in ```getRanges``` because depending on how the elements are inserted in the list, we need to lrange them differently to get them in the order we want, in other words, ```getRanges``` do not really support ascending or descending order its just telling it how to return the array items. 
For logs we put them in the list using "RPUSH", so they enter from the right side of the list. 
I also think the previous implementation of ```getLogs``` was wrong since it was not reversing the results for the descending case. So basically I think that what needs to be done is:
1) use ```multi.lrange(key, start, end);``` for the ASCENDING case, i.e. older logs first. (We consider it ascending since the log TIMESTAMP is increasing with time).
2) use ```multi.lrange(logsKey, -(end + 1), -(start + 1))```  and REVERSE the array before returning it for the DESCENDING case. i.e. newest logs first.
3) use ASCENDING as default (since the previous implementation was broken, this is actually the one that better matches the previous behaviour.

@roggervalf @tjhiggins let me know if this matches your expectations.

tjhiggins: Agreed on all 3 points. Thanks for taking the time to dig in. I'll update the bull pr to reflect. 

Patch:
diff --git a/src/classes/queue-getters.ts b/src/classes/queue-getters.ts
index 00346099a7..afa12cdcb6 100644
--- a/src/classes/queue-getters.ts
+++ b/src/classes/queue-getters.ts
@@ -128,8 +128,8 @@ export class QueueGetters extends QueueBase {
     this.commandByType(types, false, (key, command) => {
       switch (command) {
         case 'lrange':
+          multiCommands.push('lrange');
           if (asc) {
-            multiCommands.push('lrange');
             multi.lrange(key, -(end + 1), -(start + 1));
           } else {
             multi.lrange(key, start, end);
@@ -172,17 +172,27 @@ export class QueueGetters extends QueueBase {
     return Promise.all(jobIds.map(jobId => Job.fromId(this, jobId)));
   }
 
-  async getJobLogs(jobId: string, start = 0, end = -1) {
+  async getJobLogs(jobId: string, start = 0, end = -1, asc = true) {
     const client = await this.client;
     const multi = client.multi();
 
     const logsKey = this.toKey(jobId + ':logs');
-    multi.lrange(logsKey, -(end + 1), -(start + 1));
+    if (asc) {
+      multi.lrange(logsKey, start, end);
+    } else {
+      multi.lrange(logsKey, -(end + 1), -(start + 1));
+    }
     multi.llen(logsKey);
-    return multi.exec().then(result => ({
-      logs: result[0][1],
-      count: result[1][1],
-    }));
+    return multi.exec().then(result => {
+      if (!asc) {
+        result[0][1].reverse();
+      }
+
+      return {
+        logs: result[0][1],
+        count: result[1][1],
+      };
+    });
   }
 
   async getWorkers() {


Test patch:
diff --git a/src/test/test_job.ts b/src/test/test_job.ts
index f71d7958b6..11d7df90bb 100644
--- a/src/test/test_job.ts
+++ b/src/test/test_job.ts
@@ -193,7 +193,7 @@ describe('Job', function() {
   });
 
   describe('.log', () => {
-    it('can log two rows with text', async () => {
+    it('can log two rows with text in asc order', async () => {
       const firstLog = 'some log text 1';
       const secondLog = 'some log text 2';
 
@@ -203,6 +203,30 @@ describe('Job', function() {
       await job.log(secondLog);
       const logs = await queue.getJobLogs(job.id);
       expect(logs).to.be.eql({ logs: [firstLog, secondLog], count: 2 });
+      const firstSavedLog = await queue.getJobLogs(job.id, 0, 0, true);
+      expect(firstSavedLog).to.be.eql({ logs: [firstLog], count: 2 });
+      const secondSavedLog = await queue.getJobLogs(job.id, 1, 1);
+      expect(secondSavedLog).to.be.eql({ logs: [secondLog], count: 2 });
+      await job.remove();
+
+      const logsRemoved = await queue.getJobLogs(job.id);
+      expect(logsRemoved).to.be.eql({ logs: [], count: 0 });
+    });
+
+    it('can log two rows with text in desc order', async () => {
+      const firstLog = 'some log text 1';
+      const secondLog = 'some log text 2';
+
+      const job = await Job.create(queue, 'test', { foo: 'bar' });
+
+      await job.log(firstLog);
+      await job.log(secondLog);
+      const logs = await queue.getJobLogs(job.id, 0, -1, false);
+      expect(logs).to.be.eql({ logs: [secondLog, firstLog], count: 2 });
+      const secondSavedLog = await queue.getJobLogs(job.id, 0, 0, false);
+      expect(secondSavedLog).to.be.eql({ logs: [secondLog], count: 2 });
+      const firstSavedLog = await queue.getJobLogs(job.id, 1, 1, false);
+      expect(firstSavedLog).to.be.eql({ logs: [firstLog], count: 2 });
       await job.remove();
 
       const logsRemoved = await queue.getJobLogs(job.id);
@@ -497,7 +521,7 @@ describe('Job', function() {
     });
 
     it('should promote delayed job to the right queue if queue is paused', async () => {
-      const normalJob = await queue.add('normal', { foo: 'bar' });
+      await queue.add('normal', { foo: 'bar' });
       const delayedJob = await queue.add(
         'delayed',
         { foo: 'bar' },