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

issue-858 auditing timestamp supports custome date format #860

Merged
merged 4 commits into from
Dec 12, 2020

Conversation

BalloonWen
Copy link
Contributor

@BalloonWen BalloonWen commented Dec 11, 2020

  • added timestampFormat in audit.yml, if timestampFormat is specified, then will use the format,
    if the format of the timestamp is invalid, it will use a long value.
  • if the format is incorrect, it will show an error when the server starts up

- added timestampFormat in audit.yml, if timestampFormat is specified, then will use the format, otherwise will use long value
@jaswalkiranavtar
Copy link
Contributor

What will happen if user accendtly provide invalid format? Can we stop the app from starting or fallback to some default format and signal the user that you entered invalid format that is why he are seeing fallback format?

@BalloonWen
Copy link
Contributor Author

What will happen if user accendtly provide invalid format? Can we stop the app from starting or fallback to some default format and signal the user that you entered invalid format that is why he are seeing fallback format?

I can add a log if there's an invalid format, because in production, the format should be a tested one.
If we fallback to a format, it will add confusion to developer why the format doesn't change or so.
To stop the server, we could do that, but then, the server will depends on audit handler, which I don't think it's a good approach based on current structure.

What do you think?

@stevehu
Copy link
Contributor

stevehu commented Dec 11, 2020

We cannot stop the server but should output an error status code designate for this error. It should be very easy to detect using the development and testing cycle and get it fixed.

verifyAuditInfo("timestamp", "2020-12-10T17:30:11.945-0500");
}

/* @Test used for testing when doesn't specify timestampFormat
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we delete this, if this test is not being used?

@@ -358,6 +359,38 @@ public void testAuditWith401ServiceId() throws Exception {
verifyAuditInfo("serviceId", "com.networknt.petstore-1.0.0");
}

@Test
public void testAuditWith200TimestampFormatted() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it also possible to test the situation when adds the invalid format, with current solution, I would expect the app failing to start.

Copy link
Contributor Author

@BalloonWen BalloonWen Dec 11, 2020

Choose a reason for hiding this comment

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

Is it also possible to test the situation when adds the invalid format, with current solution, I would expect the app failing to start.

I will add a test for that, but the server won't stop because of the format error.
I'm trying to find a better way to set the format dynamically.

@@ -20,6 +20,8 @@ auditOnError: false
# log level; by default set to info
logLevelIsError: false

# the format for outputting the timestamp, if not specify, will use long value
Copy link
Contributor

Choose a reason for hiding this comment

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

lets also document here what would happen if the format that user entered is invalid.

- merged from master, resolved conflicts.
Bolun Wen added 2 commits December 11, 2020 19:22
- added status dependency
- create status for invalid config value
- now create formater at the server starting stage, if the provided format is invalid, it will log an error and use the default "long" style as the timestamp
- improved comment
Copy link
Contributor

@jaswalkiranavtar jaswalkiranavtar left a comment

Choose a reason for hiding this comment

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

LGTM

@stevehu stevehu merged commit 20a4789 into master Dec 12, 2020
@stevehu stevehu deleted the feat/858-audit-format branch December 14, 2020 13:17
@BalloonWen BalloonWen changed the title issue-858 issue-858 auditing timestamp supports custome date format Dec 14, 2020
younggwon1 pushed a commit to younggwon1/light-4j that referenced this pull request Feb 10, 2024
* issue-858
- added timestampFormat in audit.yml, if timestampFormat is specified, then will use the format, otherwise will use long value

* issue-858
- added status dependency
- create status for invalid config value
- now create formater at the server starting stage, if the provided format is invalid, it will log an error and use the default "long" style as the timestamp

* issue-858
- improved comment

Co-authored-by: Bolun Wen <bolun.wen@sunlife.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance the audit handler to take the timestamp pattern as parameter.
3 participants