Skip to content
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-3501 - adding new endpoint for parsing fasta file data #80

Merged
merged 5 commits into from
Feb 23, 2024

Conversation

nitin-ebi
Copy link
Contributor

No description provided.

@nitin-ebi nitin-ebi self-assigned this Feb 20, 2024
@nitin-ebi nitin-ebi changed the title EVA3501 - adding new endpoint for parsing fasta file data EVA-3501 - adding new endpoint for parsing fasta file data Feb 21, 2024
Copy link
Member

@tcezard tcezard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
I tested it locally and it works.

@@ -86,4 +87,27 @@ public Optional<Map<String, Object>> getAllPossibleSeqColExtendedData(String acc

return Optional.of(seqColResultData);
}

public Optional<Map<String, Object>> getAllPossibleSeqColExtendedData(String insdcAccession, String fastFileContent) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This being a generic function for any fasta file, I'm not sure it makes sense to put it in NCBISeqColDataSource.java

public Optional<AssemblySequenceEntity> getAssemblySequencesByAccession(String insdcAccession, String fastFileContent) throws IOException {
AssemblySequenceEntity assemblySequenceEntity;
try (InputStream stream = new ByteArrayInputStream(fastFileContent.getBytes())) {
NCBIAssemblySequenceReader reader = readerFactory.build(stream, insdcAccession);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll probably have to refactor this since NCBIAssemblySequenceReader is actually pretty generic and does not need to be attached to 'NCBI' prefix

@ApiResponse(responseCode = "500", description = "Server Error")
})
@PutMapping(value = "/seqcols/fasta/{insdcAccession}")
public ResponseEntity<?> fetchAndInsertSeqColByParsingFastFile(@PathVariable(value = "insdcAccession") String insdcAccession, @RequestBody String fastFileContent) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the string parameter is being used anywhere. It is here to comply with the current object structure.
I guess we should at least change the variant name (and the doc) to make clear it does not have to be an INSDC assembly accession.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Positive

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, it's not being used in the calculations but since we are using a lot of existing code that requires this value, I left it as it is. Was named insdcAccession, since it ultimately sets the insdcAccession value of AssemblySequenceEntity.
I have updated the parameter name to just accession.

@tcezard tcezard self-requested a review February 21, 2024 13:58
Copy link
Member

@tcezard tcezard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant to set it as "Comment"

@waterflow80
Copy link
Collaborator

Sorry, I didn't mean to commit the change, just to suggest

Copy link
Collaborator

@waterflow80 waterflow80 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
Have nothing to add on Tim's comments.

@nitin-ebi
Copy link
Contributor Author

To be discussed in stand-up

The idea for this PR was to get the functionality working without modifying too much of the existing code (easier to remove the new changes if needed).
As for refactoring, I presume we will be revisiting all these classes for major refactoring as they are plagued by the same issues that we saw in Contig Alias.

Copy link
Member

@tcezard tcezard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We decided that we will have a refactor of the code at a later date that could bring in some of the changes made in contig alias.

@nitin-ebi nitin-ebi merged commit 07a38d8 into EBIvariation:main Feb 23, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants