-
Notifications
You must be signed in to change notification settings - Fork 113
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
[AIRAVATA-2712] Refactoring App Catalog Implementation - UserResourceProfile #185
Conversation
@smarru can you review this pull request? |
…into AIRAVATA-2712
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.
For the most part looks good. I have some requested changes. Thanks Sneha!
//default serial version id, required for serializable classes. | ||
private static final long serialVersionUID = 1L; | ||
|
||
@Id |
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.
I don't think you need any JPA annotations on an id class. See Example 7-4.
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.
You're right, @machristie
@sachinkariyattin, I think we've been including JPA annotations for all Id classes.
//default serial version id, required for serializable classes. | ||
private static final long serialVersionUID = 1L; | ||
|
||
@Id |
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.
Same comment as above
private boolean validated; | ||
|
||
@ManyToOne(targetEntity = UserResourceProfileEntity.class, cascade = CascadeType.MERGE) | ||
@JoinColumn(name = "USER_ID") |
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.
This should join on USER_ID and GATEWAY_ID
private String loginUserName; | ||
|
||
@ManyToOne(targetEntity = UserResourceProfileEntity.class, cascade = CascadeType.MERGE) | ||
@JoinColumn(name = "USER_ID") |
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.
This should join on USER_ID and GATEWAY_ID
//default serial version id, required for serializable classes. | ||
private static final long serialVersionUID = 1L; | ||
|
||
@Id |
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.
Same comment as above.
|
||
@Override | ||
public UserComputeResourcePreference getUserComputeResourcePreference(String userId, String gatewayId, String hostId) throws AppCatalogException { | ||
UserComputeResourcePreferenceRepository userComputeResourcePreferenceRepository = new UserComputeResourcePreferenceRepository(); |
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.
Same comment here, should be able to do a userComputeResourcePreferenceRepository.get()
.
queryParameters.put(DBConstants.UserStoragePreference.USER_ID, userId); | ||
queryParameters.put(DBConstants.UserStoragePreference.GATEWAY_ID, gatewayId); | ||
queryParameters.put(DBConstants.UserStoragePreference.STORAGE_RESOURCE_ID, storageId); | ||
List<UserStoragePreference> userStoragePreferenceList = |
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.
Same, should do a userStoragePreferenceRepository.get()
.
} | ||
|
||
@Override | ||
public String getUserNamefromID(String userId, String gatewayID) throws AppCatalogException { |
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.
Hmm, what is this used for, this seems weird?
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.
I don't really know. This method is a part of the UsrResourceProfile interface. So, I had to override it. But it doesn't make sense.
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.
Okay, that's fine, let's keep it for now since it is part of the interface.
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.
Sure.
@@ -37,4 +37,26 @@ | |||
"WHERE BQ.groupResourceProfileId LIKE : " + DBConstants.GroupResourceProfile.GROUP_RESOURCE_PROFILE_ID; | |||
String FIND_ALL_GROUP_COMPUTE_RESOURCE_POLICY = "SELECT CR FROM "+ ComputeResourcePolicyEntity.class.getSimpleName() + " CR " + | |||
"WHERE CR.groupResourceProfileId LIKE : " + DBConstants.GroupResourceProfile.GROUP_RESOURCE_PROFILE_ID; | |||
|
|||
String GET_USER_RESOURCE_PROFILE = "SELECT DISTINCT URP FROM " + UserResourceProfileEntity.class.getSimpleName() + " URP " + |
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.
GET_USER_RESOURCE_PROFILE, GET_USER_COMPUTE_RESOURCE_PREFERENCE and GET_USER_STORAGE_PREFERENCE I don't think are needed if using the get()
repository method as mentioned above.
@@ -122,6 +122,9 @@ | |||
<class>org.apache.airavata.registry.core.entities.appcatalog.StorageInterfacePK</class> |
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.
We can remove these Id classes, right?
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.
I've removed it. It's a part of the [https://github.com//pull/167] PR.
@machristie I've made the requested changes. |
Implementation of UserResourceProfileRepository.java instead of the existing implementation class dependency.
Changes made -