galovics commented on code in PR #2304:
URL: https://github.com/apache/fineract/pull/2304#discussion_r861808789
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -54,13 +55,13 @@
import org.owasp.esapi.codecs.UnixCodec;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.jdbc.core.JdbcTemplate;
import org.springframework.jdbc.core.RowMapper;
import org.springframework.jdbc.support.rowset.SqlRowSet;
import org.springframework.stereotype.Service;
@Service
+@RequiredArgsConstructor
public class ReadReportingServiceImpl implements ReadReportingService {
private static final Logger LOG =
LoggerFactory.getLogger(ReadReportingServiceImpl.class);
Review Comment:
You can use the lombok @Slf4j annotation.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -160,14 +152,15 @@ public GenericResultsetData
retrieveGenericResultset(final String name, final St
final boolean isSelfServiceUserReport) {
final long startTime = System.currentTimeMillis();
- LOG.info("STARTING REPORT: {} Type: {}", name, type);
+ LOG.info("STARTING REPORT: {} Type: {}", name.replaceAll("[\n\r\t]",
"_"), type.replaceAll("[\n\r\t]", "_"));
final String sql = getSQLtoRun(name, type, queryParams,
isSelfServiceUserReport);
final GenericResultsetData result =
this.genericDataService.fillGenericResultSet(sql);
final long elapsed = System.currentTimeMillis() - startTime;
- LOG.info("FINISHING Report/Request Name: {} - {} Elapsed Time:
{}", name, type, elapsed);
+ LOG.info("FINISHING Report/Request Name: {} - {} Elapsed Time:
{}", name.replaceAll("[\n\r\t]", "_"),
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -160,14 +152,15 @@ public GenericResultsetData
retrieveGenericResultset(final String name, final St
final boolean isSelfServiceUserReport) {
final long startTime = System.currentTimeMillis();
- LOG.info("STARTING REPORT: {} Type: {}", name, type);
+ LOG.info("STARTING REPORT: {} Type: {}", name.replaceAll("[\n\r\t]",
"_"), type.replaceAll("[\n\r\t]", "_"));
Review Comment:
Let's do a log level check here to make sure the replacement is only
executed if the logger is configured for info level. The typical pattern here
is:
```
if (LOG.isInfoEnabled()) {
...
}
```
Also, I honestly think info is way too verbose here, let's decrease it to
debug level.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/contentrepository/S3ContentRepository.java:
##########
@@ -154,23 +154,19 @@ private void deleteObject(final String location) {
private void putObject(final String filename, final InputStream
inputStream, final String s3UploadLocation)
throws ContentManagementException {
try {
- LOG.info("Uploading a new object to S3 {}", s3UploadLocation);
+ LOG.info("Uploading a new object to S3 {}",
s3UploadLocation.replaceAll("[\n\r\t]", "_"));
this.s3Client.putObject(new PutObjectRequest(this.s3BucketName,
s3UploadLocation, inputStream, new ObjectMetadata()));
- } catch (AmazonServiceException ase) {
+ } catch (AmazonClientException ase) {
throw new ContentManagementException(filename, ase.getMessage(),
ase);
- } catch (final AmazonClientException ace) {
- throw new ContentManagementException(filename, ace.getMessage(),
ace);
}
}
private S3Object getObject(String key) {
try {
LOG.info("Downloading an object from Amazon S3 Bucket: {},
location: {}", this.s3BucketName, key);
return this.s3Client.getObject(new
GetObjectRequest(this.s3BucketName, key));
- } catch (AmazonServiceException ase) {
+ } catch (AmazonClientException ase) {
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/contentrepository/S3ContentRepository.java:
##########
@@ -154,23 +154,19 @@ private void deleteObject(final String location) {
private void putObject(final String filename, final InputStream
inputStream, final String s3UploadLocation)
throws ContentManagementException {
try {
- LOG.info("Uploading a new object to S3 {}", s3UploadLocation);
+ LOG.info("Uploading a new object to S3 {}",
s3UploadLocation.replaceAll("[\n\r\t]", "_"));
this.s3Client.putObject(new PutObjectRequest(this.s3BucketName,
s3UploadLocation, inputStream, new ObjectMetadata()));
- } catch (AmazonServiceException ase) {
+ } catch (AmazonClientException ase) {
Review Comment:
AmazonServiceException and AmazonClientException are two distinct exceptions
with no hierarchical relation. Are you sure we're not gonna need
AmazonServiceException here?
Just to be on the safe side not to break anything, I'd say let's go with
catching both in a multi-catch block like
```
} catch (AmazonClientException | AmazonServiceException ase) {
```
##########
fineract-provider/src/main/java/org/apache/fineract/useradministration/domain/AppUser.java:
##########
@@ -622,16 +622,14 @@ public void validateHasPermissionTo(final String
function, final List<String> al
public void validateHasPermissionTo(final String function) {
if (hasNotPermissionTo(function)) {
final String authorizationMessage = "User has no authority to: " +
function;
- LOG.info("Unauthorized access: userId: {} action: {} allowed: {}",
getId(), function, getAuthorities());
+ LOG.info("Unauthorized access: userId: {} action: {} allowed: {}",
getId(), function.replaceAll("[\n\r\t]", "_"),
Review Comment:
I'm getting the feeling now that there should be a utility class for
escaping log parameters.
Do you mind creating a util and reusing it?
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/contentrepository/S3ContentRepository.java:
##########
@@ -154,23 +154,19 @@ private void deleteObject(final String location) {
private void putObject(final String filename, final InputStream
inputStream, final String s3UploadLocation)
throws ContentManagementException {
try {
- LOG.info("Uploading a new object to S3 {}", s3UploadLocation);
+ LOG.info("Uploading a new object to S3 {}",
s3UploadLocation.replaceAll("[\n\r\t]", "_"));
Review Comment:
Same as for the other class.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
