-
Notifications
You must be signed in to change notification settings - Fork 2.9k
NIFI-15596 Corrected regex for FlowFile Expiration and parsed as float to allow the "FlowFile Expiration Indicator" icon to be displayed when value is a decimal without a leading integer or when leading integer is 0. #10893
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
base: main
Are you sure you want to change the base?
Conversation
|
Will review... |
mcgilman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @dan-s1! Noted one item below. Also it would be good to include some new test cases for this. Here are some test cases that could be added to the corresponding spec.
describe('isExpirationConfigured', () => {
it('should return true when expiration is a positive integer duration', () => {
const connection = { flowFileExpiration: '30 sec' };
const result = (service as any).isExpirationConfigured(connection);
expect(result).toBe(true);
});
it('should return false when expiration is zero', () => {
const connection = { flowFileExpiration: '0 sec' };
const result = (service as any).isExpirationConfigured(connection);
expect(result).toBe(false);
});
it('should return true when expiration is a decimal with leading zero', () => {
const connection = { flowFileExpiration: '0.5 sec' };
const result = (service as any).isExpirationConfigured(connection);
expect(result).toBe(true);
});
it('should return true when expiration is a decimal without leading integer', () => {
const connection = { flowFileExpiration: '.5 sec' };
const result = (service as any).isExpirationConfigured(connection);
expect(result).toBe(true);
});
it('should return false when flowFileExpiration is null', () => {
const connection = { flowFileExpiration: null };
const result = (service as any).isExpirationConfigured(connection);
expect(result).toBe(false);
});
it('should return false when flowFileExpiration is undefined', () => {
const connection = { flowFileExpiration: undefined };
const result = (service as any).isExpirationConfigured(connection);
expect(result).toBe(false);
});
it('should return false when expiration has no numeric value', () => {
const connection = { flowFileExpiration: 'sec' };
const result = (service as any).isExpirationConfigured(connection);
expect(result).toBe(false);
});
it('should return true when expiration is a large integer duration', () => {
const connection = { flowFileExpiration: '3600 sec' };
const result = (service as any).isExpirationConfigured(connection);
expect(result).toBe(true);
});
it('should return false when expiration is zero decimal', () => {
const connection = { flowFileExpiration: '0.0 sec' };
const result = (service as any).isExpirationConfigured(connection);
expect(result).toBe(false);
});
});
| const match: string[] = connection.flowFileExpiration.match(/^(\d*\.?\d+).*/); | ||
| if (match !== null && match.length > 0) { | ||
| if (parseInt(match[0], 10) > 0) { | ||
| if (parseFloat(match[0]) > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (parseFloat(match[0]) > 0) { | |
| if (parseFloat(match[1]) > 0) { |
I believe we should be referencing the capture group intentionally instead of relying on parseFloat implementation that would drop the secs portion. I understand this aspect isn't modified or new in this PR but would be good to improve here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcgilman I appreciate you giving me the sample unit tests to write. I have no experience with TypeScript so getting examples of what to write helps a whole lot. I added one test to in addition to the ones you gave me when the leading number of the decimal is greater than 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcgilman Now that the regular expression is preciser, is it even necessary to also call parseFloat?
…t to allow the "FlowFile Expiration Indicator" icon to be displayed when value is a decimal without a leading integer or when leading integer is 0.
…ed to match on the first capture group.
Summary
NIFI-15596
The change involved changing the regular expression to allow for a positive decimal number with or without a leading 0 and to parse the number as a float and not an integer since a decimal without a leading integer greater than zero will parse as zero.
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000VerifiedstatusPull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
./mvnw clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation