You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2022/05/28 11:31:26 UTC
[GitHub] [fineract] temi-ro opened a new pull request, #2339: Update ImagesApiResource.java
temi-ro opened a new pull request, #2339:
URL: https://github.com/apache/fineract/pull/2339
## Description
Should fix the NullPointerException at Base64$Encoder.encode() in ImagesApiResource from https://issues.apache.org/jira/browse/FINERACT-1224 .
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] galovics commented on a diff in pull request #2339: Update ImagesApiResource.java
Posted by GitBox <gi...@apache.org>.
galovics commented on code in PR #2339:
URL: https://github.com/apache/fineract/pull/2339#discussion_r884126020
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/api/ImagesApiResource.java:
##########
@@ -165,8 +165,13 @@ public Response retrieveImage(@PathParam("entity") final String entityName, @Pat
try {
byte[] resizedImageBytes = resizedImage.getByteSource().read();
- final String clientImageAsBase64Text = imageDataURISuffix + Base64.getMimeEncoder().encodeToString(resizedImageBytes);
- return Response.ok(clientImageAsBase64Text, MediaType.TEXT_PLAIN_TYPE).build();
+
+ if(resizedImageBytes != null){
Review Comment:
Not sure if this is right to handle this way.
I assume the resizedImageBytes is only null when there wasn't any image to resize. So I guess if we handle that use-case this way, the octet-stream branch might still fail.
I'd rather ask the question, is it reasonable to deal with these type of images?
Maybe at the beginning of this API, we shall simply put a check for this case and return an HTTP 400 as retrieving an image that doesn't exist (to some sense) is not necessarily something that makes sense for me, at least now.
Do you mind explaining the exact use-case for this extra logic?
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #2339: Update ImagesApiResource.java
Posted by GitBox <gi...@apache.org>.
vorburger commented on PR #2339:
URL: https://github.com/apache/fineract/pull/2339#issuecomment-1140334790
Build still fails, but now it's just due to what looks like a "flaky" test (@temi-ro that means a test failure NOT caused by changed you made here). I searched JIRA for it, and found FINERACT-1557 about it, which I've taken the liberty to re-open (hope that's OK). I suggest that we do not merge this PR until someone (else) has a change to fix that and we have a clean build again which is green for this and future PRs? (Flaky test failures can be very confusing for newcomers to projects.)
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #2339: Update ImagesApiResource.java
Posted by GitBox <gi...@apache.org>.
vorburger commented on PR #2339:
URL: https://github.com/apache/fineract/pull/2339#issuecomment-1140244673
@temi-ro can you please fix the code format and add the missing spaces so that it's pretty? 😄
You can either "manually" fix it, or use the command that it shows that does it for you.
Please don't hesitate to let me know if you need any help.
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a diff in pull request #2339: Update ImagesApiResource.java
Posted by GitBox <gi...@apache.org>.
vorburger commented on code in PR #2339:
URL: https://github.com/apache/fineract/pull/2339#discussion_r884130008
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/api/ImagesApiResource.java:
##########
@@ -165,8 +165,13 @@ public Response retrieveImage(@PathParam("entity") final String entityName, @Pat
try {
byte[] resizedImageBytes = resizedImage.getByteSource().read();
- final String clientImageAsBase64Text = imageDataURISuffix + Base64.getMimeEncoder().encodeToString(resizedImageBytes);
- return Response.ok(clientImageAsBase64Text, MediaType.TEXT_PLAIN_TYPE).build();
+
+ if(resizedImageBytes != null){
Review Comment:
@galovics Hey! Thanks for jumping on this review - on a Saturday no less. So, the context here is that @temi-ro is a student who I'm teaching how to FLOSS and GitHub... we picked FINERACT-1224 as an example, and this is his very first contribution. We haven't gotten to actually running Fineract, and uploading images, and fully reproducing the issue. This would take more time than we have together today. The question is if this PR as-is (once it passed the build, of course) could be acceptable to you as a first step to at least avoid the NPE and clearer diagnostic log, as a first step? Still better than the NPE, IMHO. (I may follow-up with @temi-ro to further improve this case when I show him how to actually run everything etc.)
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger merged pull request #2339: Update ImagesApiResource.java
Posted by GitBox <gi...@apache.org>.
vorburger merged PR #2339:
URL: https://github.com/apache/fineract/pull/2339
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #2339: Fix NullPointerException at Base64$Encoder in ImagesApiResource (FINERACT-1224)
Posted by GitBox <gi...@apache.org>.
vorburger commented on PR #2339:
URL: https://github.com/apache/fineract/pull/2339#issuecomment-1146096893
I've now gone ahead and merged this - after I've convinced myself that this change does not actually break that `ClientLoanIntegrationTest`, by rebasing this change on today's `develop`, and seen it build green in #2346. (So the build failure seen on this PR was either a temporary flaky test, or was meanwhile fixed.)
@temi-ro note how a project typically may not merge your 3 Git commits from this PR exactly as-is; what I did is called a "Squash and Merge" - combining your 3 separate commits into 1 new single commit, with slightly updating wording. You are still "attributed" as the author though. So congratulations on your very very first contribution to an open source project! 🥳 🎉
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] temi-ro commented on a diff in pull request #2339: Update ImagesApiResource.java
Posted by GitBox <gi...@apache.org>.
temi-ro commented on code in PR #2339:
URL: https://github.com/apache/fineract/pull/2339#discussion_r884129538
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/api/ImagesApiResource.java:
##########
@@ -165,8 +165,13 @@ public Response retrieveImage(@PathParam("entity") final String entityName, @Pat
try {
byte[] resizedImageBytes = resizedImage.getByteSource().read();
- final String clientImageAsBase64Text = imageDataURISuffix + Base64.getMimeEncoder().encodeToString(resizedImageBytes);
- return Response.ok(clientImageAsBase64Text, MediaType.TEXT_PLAIN_TYPE).build();
+
+ if(resizedImageBytes != null){
+ final String clientImageAsBase64Text = imageDataURISuffix + Base64.getMimeEncoder().encodeToString(resizedImageBytes);
+ return Response.ok(clientImageAsBase64Text, MediaType.TEXT_PLAIN_TYPE).build();
+ }else{
+ return Response.noContent().build();
Review Comment:
Done!
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] temi-ro commented on pull request #2339: Update ImagesApiResource.java
Posted by GitBox <gi...@apache.org>.
temi-ro commented on PR #2339:
URL: https://github.com/apache/fineract/pull/2339#issuecomment-1140397480
Ok! I see.
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] temi-ro commented on pull request #2339: Update ImagesApiResource.java
Posted by GitBox <gi...@apache.org>.
temi-ro commented on PR #2339:
URL: https://github.com/apache/fineract/pull/2339#issuecomment-1140294264
@galovics at the moment I tried to use the Logger, since I have never seen Lombok and I am short in time today. Moreover, I am not sure if the checks will pass since on my laptop (Windows), there are some errors in relation with the line separators...
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a diff in pull request #2339: Update ImagesApiResource.java
Posted by GitBox <gi...@apache.org>.
vorburger commented on code in PR #2339:
URL: https://github.com/apache/fineract/pull/2339#discussion_r884127164
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/api/ImagesApiResource.java:
##########
@@ -165,8 +165,13 @@ public Response retrieveImage(@PathParam("entity") final String entityName, @Pat
try {
byte[] resizedImageBytes = resizedImage.getByteSource().read();
- final String clientImageAsBase64Text = imageDataURISuffix + Base64.getMimeEncoder().encodeToString(resizedImageBytes);
- return Response.ok(clientImageAsBase64Text, MediaType.TEXT_PLAIN_TYPE).build();
+
+ if(resizedImageBytes != null){
+ final String clientImageAsBase64Text = imageDataURISuffix + Base64.getMimeEncoder().encodeToString(resizedImageBytes);
+ return Response.ok(clientImageAsBase64Text, MediaType.TEXT_PLAIN_TYPE).build();
+ }else{
+ return Response.noContent().build();
Review Comment:
`serverError()` seems more appropriate than `noContent()` in this particular case, I think
Also, let's log the fact that this happenend...
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] galovics commented on pull request #2339: Update ImagesApiResource.java
Posted by GitBox <gi...@apache.org>.
galovics commented on PR #2339:
URL: https://github.com/apache/fineract/pull/2339#issuecomment-1140292984
> @temi-ro The build is still failing, probably because it's missing the import for the Logger. Do you still want to fix that? Then I can merge this, as your very first (real, useful, actually fixing a small bug in a real world open source project) Pull Request!
@vorburger @temi-ro you guys could even use Lombok there with the `@Slf4j` annotation, then no need to explicitly create the logger.
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a diff in pull request #2339: Update ImagesApiResource.java
Posted by GitBox <gi...@apache.org>.
vorburger commented on code in PR #2339:
URL: https://github.com/apache/fineract/pull/2339#discussion_r884125591
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/api/ImagesApiResource.java:
##########
@@ -165,8 +165,13 @@ public Response retrieveImage(@PathParam("entity") final String entityName, @Pat
try {
byte[] resizedImageBytes = resizedImage.getByteSource().read();
- final String clientImageAsBase64Text = imageDataURISuffix + Base64.getMimeEncoder().encodeToString(resizedImageBytes);
- return Response.ok(clientImageAsBase64Text, MediaType.TEXT_PLAIN_TYPE).build();
+
Review Comment:
could you also remove this empty line?
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #2339: Update ImagesApiResource.java
Posted by GitBox <gi...@apache.org>.
vorburger commented on PR #2339:
URL: https://github.com/apache/fineract/pull/2339#issuecomment-1140275301
@temi-ro The build is still failing, probably because it's missing the import for the Logger. Do you still want to fix that? Then I can merge this, as your very first (real, useful, actually fixing a small bug in a real world open source project) Pull Request!
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] galovics commented on a diff in pull request #2339: Update ImagesApiResource.java
Posted by GitBox <gi...@apache.org>.
galovics commented on code in PR #2339:
URL: https://github.com/apache/fineract/pull/2339#discussion_r884130952
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/api/ImagesApiResource.java:
##########
@@ -165,8 +165,13 @@ public Response retrieveImage(@PathParam("entity") final String entityName, @Pat
try {
byte[] resizedImageBytes = resizedImage.getByteSource().read();
- final String clientImageAsBase64Text = imageDataURISuffix + Base64.getMimeEncoder().encodeToString(resizedImageBytes);
- return Response.ok(clientImageAsBase64Text, MediaType.TEXT_PLAIN_TYPE).build();
+
+ if(resizedImageBytes != null){
Review Comment:
Sure thing. Thanks for the context @vorburger.
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org