Merge from BI Server by jloux-brapi · Pull Request #2 · plantbreeding/brapi-Java-ProdServer

jloux-brapi · 2024-12-17T21:58:27Z

No description provided.

some initial performance experiments including indexing and batching database operations

added IF NOT EXISTS to CREATE INDEX statements

…nd how performance can be improved for fetching OUs, Observations, and ObsVars

… searches Also did some code cleanup, and added a logback.xml config file

fixed NPE

… problem. It was not needed. The ScaleVlalidValueCategories were present (even though it is set to fetch LAZY) because of the SQL like statemtents called on searchQuery

…leves

…rvice.java

…amples

…ocker Create local containerized keycloak docker example, update README

jloux-brapi

There's a lot of stuff here. I'm hoping me, Pete, and BI devs who worked on this (or whoever is interested) can have a meeting in the new year and go through all these points together so we can start bucketing the stuff we want to keep, change, etc.

I also made some comments pertaining to the code. I'm not really expecting these to change or anything at this point, just pointing them out and want to see if we can refactor or change at some point.

jloux-brapi · 2024-12-18T19:13:59Z

src/main/java/org/brapi/test/BrAPITestServer/service/pheno/ObservationUnitService.java

+		if(includeObservations) {
+			log.debug("Fetching observations for OUs");
+			for(ObservationUnitEntity entity : page) {
+				log.trace("Fetching observations for OU: " + entity.getId());
+				entity.getObservations();


We are calling an accessor just to do go around the lazy loading? If so, this to me is not really the intent of what lazy loading should be used for. Lazy loading is to only fetch data when it is accessed by the app for business purposes. What this method appears to be doing though is returning a data set directly to a user, and actually in fact has very little business logic. I feel like instead of working around the lazy loading we should create a service/repo method to actually grab the data the user needs in one go. Doing it as written seems to create more DB connections than are necessary to carry out this transaction.

If in fact we expect to load this on a given record fetch (which is what this method seems to be), I'd argue the Observation entity attached to ObservationUnit should be eagerly loaded, not lazily loaded, depending on if there are other business use cases.

I believe the business logic here is that includeObservations is a rare case. 90% of the time includeObservations will be false and this will avoid much of the accessor and mapping code needed to return Observations.

I agree with you that this could be redone with a more direct call to the database to gather the observations needed, rather than relying on Hibernate to load them.

Not BI specific logic, merge this and mark it for potential rework later

src/main/java/org/brapi/test/BrAPITestServer/service/pheno/ObservationUnitService.java

jloux-brapi · 2024-12-18T19:31:32Z

src/main/java/org/brapi/test/BrAPITestServer/service/pheno/ObservationService.java

+		if(!page.isEmpty()) {
+			observationRepository.fetchXrefs(page, ObservationEntity.class);
+		}


Why is this not in its own separate code path? Does it really need to depend on an already expensive query to determine its execution? Or can we create some diverging codepath or endpoint where we can execute this query independently. Need to know more about the use case here.

I believe this is because Xrefs is actually part of the parent object, so for some reason it can't be accessed by the leftJoinFetch query builder above? so it has to be fetched after the fact, forcing an eager loading here, instead of loading each observation individually later when using the get methods.

It seems to me that if we need to avoid the lazy loading in a particular case, then it should all be done one way. Either all through the LEFT JOIN FETCH query, or using Hibernates own accessors like here....

Not BI specific, merge and mark for review with the rest of the lazy vs eager loading code

Resolving due to revelations in this comment: #2 (comment)

src/main/java/org/brapi/test/BrAPITestServer/service/pheno/ObservationService.java

jloux-brapi · 2024-12-20T20:37:47Z

src/main/resources/db/migration/V002.004__add_soft_deleted_column_to_list.sql

+CREATE OR REPLACE FUNCTION sync_list_related_tables_soft_deleted()
+RETURNS TRIGGER AS $$
+BEGIN
+    -- Update list_external_references
+UPDATE public.list_external_references
+SET soft_deleted = NEW.soft_deleted
+WHERE list_entity_id = NEW.id;
+
+-- Update list_item
+UPDATE public.list_item
+SET soft_deleted = NEW.soft_deleted
+WHERE list_id = NEW.id;
+
+RETURN NEW;
+END;
+$$
+LANGUAGE plpgsql;
+
+-- Create a trigger on the list table
+CREATE TRIGGER sync_soft_deleted_status
+    AFTER UPDATE OF soft_deleted ON public.list
+    FOR EACH ROW
+    WHEN (OLD.soft_deleted IS DISTINCT FROM NEW.soft_deleted)
+EXECUTE FUNCTION sync_list_related_tables_soft_deleted();


I'm gonna need a walkthrough on the purpose and need of this to determine if we need this in the prod server.

... same ...

This SQL migration script creates a trigger that synchronizes the soft_deleted status across related tables when a list is soft-deleted or restored. Here's a breakdown of its purpose:

Trigger Function:
The script defines a function called sync_list_related_tables_soft_deleted() that performs the following actions:

Updates the soft_deleted status in the list_external_references table for all rows related to the updated list.

Updates the soft_deleted status in the list_item table for all rows related to the updated list.

Trigger Creation:
The script then creates a trigger named sync_soft_deleted_status on the public.list table with the following characteristics:

It fires AFTER an UPDATE operation on the soft_deleted column of the list table.

It executes FOR EACH ROW affected by the update.

It only fires WHEN the soft_deleted status has actually changed (OLD.soft_deleted IS DISTINCT FROM NEW.soft_deleted).

The purpose of this trigger is to maintain data consistency across related tables. When a list is soft-deleted or restored (by changing its soft_deleted status), the trigger ensures that all related records in the list_external_references and list_item tables are updated accordingly.

jloux-brapi · 2024-12-20T20:39:15Z

src/main/resources/db/migration/V002.004__add_soft_deleted_column_to_list.sql

+ALTER TABLE public.list
+    ADD COLUMN soft_deleted BOOLEAN NOT NULL DEFAULT FALSE;
+
+ALTER TABLE public.list_external_references
+    ADD COLUMN soft_deleted BOOLEAN NOT NULL DEFAULT FALSE;
+
+ALTER TABLE public.list_item
+    ADD COLUMN soft_deleted BOOLEAN NOT NULL DEFAULT FALSE;


archived much better name/term than soft_deleted IMO.

If clients could specify that they wanted archived data in responses with a parameter, I think archived makes more sense, however, if the soft-delete is transparent to the client, and soft-deleted data is never returned in responses, I think soft_deleted is a good name.

I thought the soft-delete approach was basically to ease concerns around user error causing data loss, but it would be great to have a documented objective or rationale for delete functionality in the BrAPI spec (assuming it eventually goes there).

jloux-brapi · 2024-12-20T20:44:31Z

src/main/resources/db/migration/V002.005__cascade_delete_sample.sql

+-- First, drop the existing foreign key constraint
+ALTER TABLE ONLY public.vendor_file_sample
+DROP CONSTRAINT IF EXISTS fke3tnyn895kve2kgixku4j7htb;
+
+ALTER TABLE ONLY public.callset
+DROP CONSTRAINT IF EXISTS fkhreq22htrftm3dul7nfsg1agk;


We should definitely consider keeping these cascades to the main branch of the prod server.

jloux-brapi · 2024-12-20T20:47:15Z

src/main/java/org/brapi/test/BrAPITestServer/repository/core/ListRepository.java

 public interface ListRepository extends BrAPIRepository<ListEntity, String>{
+    public Page<ListEntity> findAllBySearchAndNotDeleted(SearchQueryBuilder<ListEntity> searchQuery, Pageable pageReq);
+
+    @Query("SELECT l FROM ListEntity l WHERE l.id = :id AND l.softDeleted = false")


Definitely think we want to keep this functionality (and similar elsewhere) in the prod server base branch.

jloux-brapi · 2024-12-20T21:01:36Z

src/main/java/org/brapi/test/BrAPITestServer/repository/core/TrialRepository.java

+	@Query("UPDATE TrialEntity t SET t.softDeleted = :softDeleted WHERE t.id = :trialId")
+	int updateSoftDeletedStatus(@Param("trialId") String trialId, @Param("softDeleted") boolean softDeleted);


I'm starting to get the feeling these soft queries could be generified. Not sure why we need to define them in every entity that needs them. I suppose maybe it's bc not every entity necessarily has this column, but I think we could employ some kind of repo inheritance for entities that need deletion, this repo could extend from BrAPIRepository, and it contains these soft deleted methods generified for any entity that needs them.

I agree that any common definitions like this should be placed in a parent class or interface when adding delete functionality across all primary BrAPI entities. The only reason it was repeated for distinct implementations or classes here is that delete functionality was only being implemented for three primary BrAPI entities instead of across all primary entities.

jloux-brapi · 2025-01-08T23:35:32Z

Per @BrapiCoordinatorSelby I am merging this. It seems most of the code in this MR we do want in the prod branch, however we will have to make some new MRs to get code we don't want out and to refactor some things.

We can do this by making issues of some of the more important comments that exist.

mlm483 and others added 30 commits December 17, 2024 16:27

[BI-1771] - performance experiments

aa958f9

some initial performance experiments including indexing and batching database operations

[BI-1771] - added germplasm-related indexes

d370995

[BI-1771] - reverted index annotation

853c4b6

[BI-1771] - reverted whitespace changes

1bafb97

[BI-1771] - made create_indexes.sql idempotent

511744a

added IF NOT EXISTS to CREATE INDEX statements

[BI-1909] Exploring converting *ToOne relationships to lazy loading a…

17f208b

…nd how performance can be improved for fetching OUs, Observations, and ObsVars

[BI-1909] NPE fix, converting sysouts to use Logger

65f41bb

[BI-1909] Updating GermplasmService to eagerly fetch related date for…

f213636

… searches Also did some code cleanup, and added a logback.xml config file

[BI-1909] suggested changes

444ac44

Create pull.yml

7ae834a

[BI-1945] - saving work in progress

f3963b0

[BI-1945] - store additionalInfo on primary entities as JSONB

14347ea

[BI-1945] - optimized imports

0a6c1c6

[BI-1945] - updated template, readme

dfe5e82

[BI-1945] - cleaned up SQL dummy data migrations

32a1c91

[BI-1945] - updated template

c31973b

[BI-1945] - removed unused class

3d73553

[BI-1945] - renamed method

213d51b

[BI-1945] - handled null case

ea38550

[BI-1945] - added stringtype=unspecified to template

14d780c

[BI-1945] - removed debug log

027eff8

[BI-1945] - optimized imports

2e57478

[BI-1945] - removed additionalInfo joins

246e6a1

fixed NPE

[BI-1945] - updated README.md

44c6a6a

[BI-1945] - added ON DELETE CASCADE to xref linking tables

4d25468

[BI-2051] - batched inserts for efficiency

530ae23

[BI-2051] - removed comment

1777923

[BI-2040] The fetchScaleValidValueCategories() method was causing the…

9bb3096

… problem. It was not needed. The ScaleVlalidValueCategories were present (even though it is set to fetch LAZY) because of the SQL like statemtents called on searchQuery

[BI-2040] removed all calls to fetchXXXXX(page) and the methods thems…

dc3aa96

…leves

[BI-2040] removed unused import-statements from ObservationVariableSe…

f3c8871

…rvice.java

dmeidlin and others added 24 commits December 17, 2024 16:47

json deserialize searchrequest sub-types

92aa47f

return batchDbID in POST batches response results

7b6b61a

refactor

e74184b

add constraint to cascade on delete for list_item

5c00c3d

add soft_deleted column to list tables

beb95c4

modify list repository to include clause softDelete=falsee

9ebbfb1

use where clause and transactional annotations

ed8ee46

soft-delete batch lists

0c38c55

add hard delete of single trial

0191666

create hard-delete endpoint for single sample

0547cbc

cascade delete for trial and sample

d659040

add soft_delete columns to trial and sample related tables

0d4ac86

create sample service soft-delete method

fc10679

add soft-delete method to trial service

aa8b86f

create Trial and Sample components

a5d8656

optimize imports

b96e1c9

respond with 204 for successful delete

f2036bf

return entity dbids in response for POST deleteBatch

cbdd0f3

add batchDeleteDbId query param to GET endpoints for lists, trials, s…

493f88f

…amples

create new exception class for wrong batch delete type

6fa2c21

delete batch entity when deleting batch contents

71cdbb1

add cascade delete constraints for study foreign keys

bd2b039

rename batch to batch delete

0efca81

Solve dummy data migration issues

b57a17d

jloux-brapi mentioned this pull request Dec 17, 2024

Merge from BI server #1

Closed

Merge pull request plantbreeding#75 from plantbreeding/keycloak-dev-d…

b7ecfda

…ocker Create local containerized keycloak docker example, update README

jloux-brapi commented Dec 20, 2024

View reviewed changes

jloux-brapi merged commit 75496f3 into brapi-server-v2 Jan 8, 2025

jloux-brapi deleted the bi-server-impl branch January 8, 2025 23:36

		@Query("UPDATE TrialEntity t SET t.softDeleted = :softDeleted WHERE t.id = :trialId")
		int updateSoftDeletedStatus(@Param("trialId") String trialId, @Param("softDeleted") boolean softDeleted);

Conversation

jloux-brapi commented Dec 17, 2024

Uh oh!

jloux-brapi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jloux-brapi commented Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jloux-brapi commented Jan 8, 2025 •

edited

Loading