-
Notifications
You must be signed in to change notification settings - Fork 21
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
EVA-3475 - Release count endpoints v2 #177
EVA-3475 - Release count endpoints v2 #177
Conversation
75ee6fa
to
4f01662
Compare
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 guess there's some updates to be made to support the lists of taxonomies/assemblies, but here are my comments on the rest (mostly naming things)
eva-release/src/main/java/uk/ac/ebi/eva/release/controllers/ReleaseStatsv2Controller.java
Outdated
Show resolved
Hide resolved
eva-release/src/main/java/uk/ac/ebi/eva/release/controllers/ReleaseStatsv2Controller.java
Outdated
Show resolved
Hide resolved
eva-release/src/main/java/uk/ac/ebi/eva/release/controllers/ReleaseStatsv2Controller.java
Outdated
Show resolved
Hide resolved
eva-release/src/main/java/uk/ac/ebi/eva/release/dto/ReleaseStatsPerSpeciesV2Dto.java
Outdated
Show resolved
Hide resolved
eva-release/src/main/java/uk/ac/ebi/eva/release/models/ReleaseStatsView.java
Outdated
Show resolved
Hide resolved
|
||
import org.hibernate.annotations.Type; | ||
|
||
import javax.persistence.*; |
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.
Should avoid wildcard imports
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.
In case IntelliJ puts a wildcard when you format using the shortcut, you can change the default value in
File -> Settings -> Editor -> Code -Style -> Java -> Imports tab
- Class count to use import with '*': Set this value to something like 50 (default is 5)
eva-release/src/main/java/uk/ac/ebi/eva/release/services/ReleaseStatsServicev2.java
Outdated
Show resolved
Hide resolved
eva-release/src/main/java/uk/ac/ebi/eva/release/dto/ReleaseStatsPerV2Dto.java
Outdated
Show resolved
Hide resolved
eva-release/src/main/java/uk/ac/ebi/eva/release/dto/ReleaseStatsPerAssemblyV2Dto.java
Outdated
Show resolved
Hide resolved
eva-release/src/main/java/uk/ac/ebi/eva/release/dto/ReleaseStatsPerAssemblyV2Dto.java
Outdated
Show resolved
Hide resolved
eva-release/src/main/java/uk/ac/ebi/eva/release/dto/ReleaseStatsPerV2Dto.java
Outdated
Show resolved
Hide resolved
|
||
public class ReleaseStatsPerSpeciesV2Dto extends ReleaseStatsPerV2Dto { | ||
|
||
@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.
@Id |
|
||
public class ReleaseStatsPerAssemblyV2Dto extends ReleaseStatsPerV2Dto { | ||
|
||
@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.
@Id |
@Id | ||
private String assemblyAccession; | ||
|
||
@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.
@Id |
package uk.ac.ebi.eva.release.dto; | ||
|
||
import javax.persistence.Id; | ||
import java.util.Objects; |
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.
unused imports
@Id | ||
private String assemblyAccession; | ||
|
||
private String rsType; |
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.
private String rsType; | |
@Id | |
private String rsType; |
If you don't intend this to be part of ID, you should probably remove it from ReleaseStatsPerAssemblyViewPK as well
private Long count; | ||
|
||
@Column(name="new") | ||
protected Long newAddition; |
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.
protected Long newAddition; | |
private Long newAddition; |
private int taxonomyId; | ||
@Id | ||
private int releaseVersion; | ||
private String rsType; |
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.
private String rsType; | |
@Id | |
private String rsType; |
If you don't intend this to be part of ID, you should probably remove it from ReleaseStatsPerAssemblyView as well
private Long count; | ||
|
||
@Column(name="new") | ||
protected Long newAddition; |
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.
protected Long newAddition; | |
private Long newAddition; |
if (!keyToDto.containsKey(key)) { | ||
keyToDto.put(key, new ReleaseStatsPerSpeciesV2Dto()); | ||
} | ||
ReleaseStatsPerSpeciesV2Dto dto = keyToDto.get(key); |
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.
if (!keyToDto.containsKey(key)) { | |
keyToDto.put(key, new ReleaseStatsPerSpeciesV2Dto()); | |
} | |
ReleaseStatsPerSpeciesV2Dto dto = keyToDto.get(key); | |
ReleaseStatsPerSpeciesV2Dto dto = keyToDto.computeIfAbsent(key, k -> new ReleaseStatsPerSpeciesV2Dto()); |
Optional
Change PK to be embeded to enable join with taxonomy Join with evapro.taxonomy to get scientific name and common name
Co-authored-by: April Shen <april.tuesday@gmail.com>
Please take a new look now that we're using the wide version of the aggregate table |
import java.util.Objects; | ||
|
||
@Entity | ||
@IdClass(ReleaseStatsPerAssemblyV2PK.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.
Should this be an @EmbeddedId
as in the taxonomy model? (I'm not sure I understand the reason to use one form vs. another, just wondering if they should be the same.)
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 mostly copied the old data model way but it was incompatible with joining with Taxonomy which is why I switch it to using @EmbeddedId
.
You're right though that we should keep them consistent so I updated this class to use @EmbeddedId
as well.
eva-release/src/main/java/uk/ac/ebi/eva/release/mappers/ReleaseStatsPerAssemblyMapper.java
Outdated
Show resolved
Hide resolved
eva-release/src/main/java/uk/ac/ebi/eva/release/dto/ReleaseStatsV2Dto.java
Outdated
Show resolved
Hide resolved
this.releaseInfoRepository = releaseInfoRepository; | ||
} | ||
|
||
public Map<Integer, String> getReleasesFtp() { |
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.
Since this is a util class, It will be better to have this as a method with a parameter (can also be static)
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 think I was trying to find a way to provide the FTP as a singleton so that we're not making the same query to the database too many time.
There might be a better way of doing that.
Co-authored-by: nitin-ebi <79518737+nitin-ebi@users.noreply.github.com> Co-authored-by: April Shen <april.tuesday@gmail.com>
No description provided.