Are you deploying this to AWS? And if the answer
is yes, then all of
angle. So basically the Access key ID as well as the Secret ID.
What's going on guys? Welcome to Amigoscode.
In this video I want to walk you through this pull
request review by one of my students. I've seen few
things which I felt maybe it's worth for you to know because
the goal with this series is for you to learn how to
write better code. Now if you're new to my channel,
literally just take 1 second subscribe. Also if you enjoyed these
kind of videos, smash the like button so I can keep on bringing you
these type of contents because it's all about giving you with
the best advice and also discuss with you guys, right? So I've been
learning from you guys and also you've been learning. So sometimes there are
things that I say and people say, oh, you could do this this way or
another way, which is really good for all of us to benefit. The other
thing that I want to say is currently the program. So this
is part of the Amigos Code Mentorship program where
we go through ten weeks as a group and we build software together.
So currently it's basically fully booked. So we are running
the second cohort, but as soon as the third cohort
opens up, I will let you know. So we are finding a way of
scaling things. Maybe we should have two, three cohorts running at the
same time, but at the moment I will let you know very soon.
So the pull request that I have here is by,
I'm not even going to mention his name, so why?
I don't want him to think that I'm picking on him,
but instead I'm just giving you feedback so that you can
improve yourself as an engineer. So this pull request
here has 18 changes. And the goal
of this pull request here, it's Media Saving
in here. So the title is called Media Saving.
So maybe here it's not
really Media Saving, but it's more like Files
or Saving files to S three or S Three
file integration. So work on the naming for
your titles. Okay, now let's navigate
and start from the top here. So here we have the palm XML.
So these are the dependencies. So here you have AWS
Java SDK, version one point whatever.
And right now I can tell you that this is wrong.
Okay, so they have the version two.
So version one actually is deprecated and you should migrate to version two.
So use V two in
here. So I'm just going to start the review.
There we go. And also I don't think that
this is needed. So spring Cloud starter.
AWS. So maybe not
needed, but I'll see in your code whether you
actually use this. Okay, so let's move on.
So here we kind of have the controllers.
So here image controller.
So I'm not going to go deep into the API
stuff, which we've done before, but here, so we should
say final. And one cool thing actually here is
that we can basically add a suggestion. So here I'm going to say
private, final and then add review. And then you
could just accept it. So you can see here, right? And then the same
for this guy right here. So this should be
private and then final just like
that. And we have the
post mapping in here. So this is post mapping customer ID upload
and we take the customer ID plus the image.
And this right here is actually incorrect.
So don't throw so here basically what you're doing is you're
letting the framework decide on how to handle the exception,
right? So you want to upload an
image, but this is an I O exception. So surely
that you are sending the image. Yeah, so this
is incorrect, okay? Also, so here, remember the
Nt architecture, so don't do this,
okay? So don't do this. Don't get the customer.
And then upload in here. So all of this should be part of the service
class. Okay? Move this logic
to service class. And I'm not going to spend
30 minutes on this pull request, but instead I'm just going to give you
the main points, okay? So that's
it. So basically upload a file for customer. And then here when
you get the image, you say Media ID,
right? So I think you're mixing few things. So you're saying
image and media. So just stick with one.
Okay, so here we have get image
and the response entity is an S three object.
So that this is wrong, okay? So get image. So this
should actually return a byte array, the image itself.
Now if you look into the S three object, s three
object will contain a bunch of stuff. Let me
show you. So in here you can see that this is the class
S three object. And have a look. So the method
summary close. Get bucket name, get key,
get object content. So this is exactly
what you want, right? So you are basically sending a bunch of stuff
to the client which it's not necessarily
needed, okay? So this is actually wrong.
So this should be the byte array and then you can get the actual contents
from it. Okay, so let's scroll down in here.
So same thing here. So this will be final and then
S three client and then the bucket name
from the environment variables. So I think this is also wrong
because your service class now
can only work upon one bucket, right? So instead you
should change this so that it's more flexible, right? So you should be able to
pass a bucket that you are working with,
right? And also maybe extract
this right here into its own class so
that other service classes can use it. Because right now
it's so specific to one bucket.
Okay, so this needs some work.
Then basically here upload image. So we
generate the key. Then we save to S three.
And basically if there is an exception, you throw
back, right? So you throw an IO
exception which is fine. So I think you're doing this because of
save to image or actually save to S three.
So this guy right here, so this I
think s three client put object. So this throws
an exception, right? So you better basically handle
the exception right here. And then depending on the response
status that you get back from the API call, you can
translate that into your own exception. Okay, so never
throw this right here. So this is wrong.
Okay. So here you save and then you create the media
and you set the UID, the type and
the customer in here. Okay, cool.
And then you save just like that.
And in here you are
getting few things from the image. So the size, the content type,
if you are saving images from
the content types, from the image, then check if it's
an image, right? So check whether this is an image
and if it's not an image then don't store, right? And then
you can basically throw an exception. Say invalid content type
for example. Okay, cool.
I'm not going to leave the comments here because it's just going to drag this
video. Then you have get image in here.
So you pass the UID, so you get the media and
then or else throw illegal argument exception.
So this I think it will resemble a 500.
So you need to be careful and maybe like say resource
not found. And then have a custom status code sent to the
client. And then you basically say
S three and then get right. But here again you're throwing
right. So it might not exist in your database.
But also what about if it doesn't exist within S three? Okay, what about
if it doesn't exist within S three? So you have to handle
this scenario as well. Okay, so try and catch in here
and then depending on the status code that you get back from the API
call that you perform with S three client,
then you provide the right status code back
to the client. Cool. So again this
is probably not needed because you should refactor this
so that it works against multiple packets and not just one specific
packet. All right, cool. So then
s three config. Now the problem with
this in here, I've got one question to you,
right? Are you deploying this to AWS?
And if the answer is yes, then all
of angle. So basically the access key ID as well as the secret
ID,
right? Because you can use IAM roles
to basically check whether the
instance so the AWS or easy to instance
has the correct permissions to interact with your bucket.
And also so here basic AWS credentials.
So this is not needed if you are deploying to AWS
because you can get the credentials from the instance profile.
Okay, so this can go and this
also can go if you're not deploying within AWS. Now if you are deploying
outside AWS, this is when you need this.
But even though if you are deploying outside AWS,
then I would suggest you to use the content
store provided by the cloud provider instead of basically
from let's say Google Cloud trying to store to AWS,
which doesn't really make sense and I'm pretty sure that you are deploying this to
AW. You are going to deploy this to AWS so this can
go. Okay? And let me quickly show you on the docs.
So in here I've got the AWS SDK for Java and you can
see that basically version one and then there's version two
and there's a migration guide. Okay? So migrate your code to version
two. So if we go into the developer guide in
here we should see a section on I
think it's use the SDK,
provide temporary credentials. And in
here you can see that they have something called the default credentials
provider chain. And have a look. So in
here so they have the order, okay, so this is the order
java system properties, environment variables, which is the
one that you are using. And then you have web identity.
Yada yada. And then the one that I was saying to you is in
here. So Amazon EC two instance role
basically, right? So the SDK uses the instance profile credentials
provider to load
temporary credentials from the Amazon EC two metadata
service. Okay? So basically it tries all of these and
basically I think yeah. So you are using this, which I
don't recommend. Okay. Because these are secrets and you don't want
to have secrets laying around
because if anyone steals the secrets then off you go.
Okay, cool. So hopefully you've
learned something new here. Then let me just scroll down
and in here I can see that you have this generic controller.
So generic controller, I'm not really sure why. I've seen
people have an abstract class with generic controller.
I'm not entirely sure if you need this. Right? So the
simpler the code the better. Okay. Because here I think all you're
doing is you're passing the data message status code
and then you're passing the timestamp and I think
that's pretty much it, right? So I don't think this
is adding any value in here. Let me just scroll down
and see. So here a couple of changes. So this is part
of the refactor that you're doing, I think. And then
you have the media in here, the type blah
blah blah. You have the media repository.
All right. So I think we'll leave it
what is this? Right? So we spoke about this
in here. So AWS s three and then probably this
would be file uploads bucket
for example, right? Not just bucket because you could
have multiple buckets within AWS S three or within S three,
and then string reg
X. I haven't seen this, but I
don't know. This is like an email validator,
so have better names. Okay, so email
validation, reg X, something like that,
I'm not really sure what is that? And then you have tests,
so image controller tests. So here you are testing
the controller, and one
thing which I can see which is wrong is that you
are mocking. So controllers, you should never mock within
controllers. Okay, so there's ways that you can actually test by
mocking the S Three implementation, right? And because you
basically haven't abstracted things well enough, it's impossible for
you to do it right now. Okay,
right, so this is all wrong. And then this is the service
which in here cool, so that's fine. Using mocks,
that's absolutely fine. And that's it cool.
So hopefully you've learned something new. And I'll
leave a link, link for the video where I show you how to implement this
the right way. Go and check it out.
And if you enjoyed this video, thumbs up. Also, if you
are new to the channel, subscribe and let me know what other videos you
want to see next. It's all for now, catch me on the next one.
Assalamualaikum.