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.

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

Removing a day of week and then adding it again results in duplicate values exception #268

Closed
Jackman3005 opened this issue May 24, 2022 · 3 comments · Fixed by #295
Closed
Assignees
Labels

Comments

@Jackman3005
Copy link

Hello,

I am trying to use cron-parser in a UI environment where the user can toggle days of the week on and off. The result of the UI is a cron string that can then be sent and stored in the API.

I am parsing a cron string (parseExpression), breaking it into its fields then adjusting fields and recreating the cron string (fieldsToExpression).

There seems to be an issue when doing this with dayOfWeek (maybe dayOfMonth as well, but I'm not working with that yet). See my code for reproduction and the corresponding output below:

Code

const expression1 = parseExpression("* * * * *");
console.log("expression1.stringify(): ", expression1.stringify());
console.log("expression1.fields.dayOfWeek: ", expression1.fields.dayOfWeek);

const dayOfWeek1 = [...expression1.fields.dayOfWeek];
dayOfWeek1.splice(dayOfWeek1.indexOf(5), 1); // remove 5 from the array
const fields1 = {
  ...expression1.fields,
  dayOfWeek: dayOfWeek1,
};
const expression2 = fieldsToExpression(fields1);
console.log("expression2.stringify(): ", expression2.stringify());

const expression3 = parseExpression(expression2.stringify());

console.log("expression3.fields.dayOfWeek: ", expression3.fields.dayOfWeek);
console.log("expression3.stringify(): ", expression3.stringify());

const dayOfWeek2 = [...expression3.fields.dayOfWeek, 5]; // add 5 back into the array
const fields2 = {
  ...expression1.fields,
  dayOfWeek: dayOfWeek2,
};

console.log("fields2: ", fields2);
const expression4 = fieldsToExpression(fields2); // fails here!
console.log("expression4.fields.dayOfWeek: ", expression4.fields.dayOfWeek);
console.log("expression4.stringify(): ", expression4.stringify());

Output

expression1.stringify():  * * * * *
expression1.fields.dayOfWeek:  [
  0, 1, 2, 3,
  4, 5, 6, 7
]
expression2.stringify():  * * * * 0-4,6,7
expression3.fields.dayOfWeek:  [
  0, 0, 1, 2,
  3, 4, 6
]
expression3.stringify():  * * * * 0,0-4,6
fields2.dayOfWeek:  [
  0, 0, 1, 2,
  3, 4, 6, 5
]

Error

Error: Validation error, Field dayOfWeek contains duplicate values
    at Function.fieldsToExpression (/Users/me/workspace/my-project/app/node_modules/cron-parser/lib/expression.js:975:13)
    at fieldsToExpression (/Users/me/workspace/my-project/app/node_modules/cron-parser/lib/parser.js:53:25)

Analysis

From what I can tell, the original expression's fields is where the issue begins. There are only 7 days in the week, yet the fields.dayOfWeek array contains 8 items. This is likely to support cron strings that use 7 for Sunday.

After removing 5 from the dayOfWeek array the stringified expression contains both 0 and 7. This is likely where the fix should occur. I don't know which situation where we would want both 0 and 7 in a stringified cron expression.

Let me know how I can help get this resolved. If you can point me towards the problem code, I'm happy to get a PR out to fix it.

Thanks in advance,
Jack

@harrisiirak harrisiirak self-assigned this May 24, 2022
@harrisiirak
Copy link
Owner

Hi @Jackman3005! I'll check it when I'm back from the holiday.

@Jackman3005
Copy link
Author

Okay, thanks for the quick reply :)

@Jackman3005
Copy link
Author

Thinking back, it makes my job a bit more difficult receiving both 0 and 7 back from the fields.dayOfWeek array. I have to check if either of these are here to make a decision on whether a job should be run on Sunday. Would be easier if it was always 0-6 in the output of cron-parser APIs but it supported 7 as an input for the scenarios where that's relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants