feat: Implement search logic for business partner extension

[kunyao-cofinity-x]

Description

• What does this PR introduce?

This feature aims to introduce enhanced search functionality for Business Partners in the CX network. It improves the current available options for identifying Business Partners in different scenarios like e.g., within the CX-onboarding process.
The benefit of the new functionality is that a member can search for a business partner based on several characteristics and receives the latest information out of the network.

• Does it add a new feature?

It is a new feature, but it is partially based on an existing data model.

Pre-review checks

Please ensure to do as many of the following checks as possible, before asking for committer review:

• DEPENDENCIES are up-to-date. Dash license tool. Committers can open IP issues for restricted libs.
• Copyright and license header are present on all affected files

[sonarqubecloud]

Quality Gate passed

Issues
42 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[nicoprow]

@kunyao-cofinity-x

A first feedback to the pull request:
There is now a lot of additional filters that can be combined with business partner searches. And these filters also include variant search clauses and and transforms like making everything lower case. With these very powerful search I could imagine that the performance might take a drastic hit on large data sets. Did you do a performance test
on your data set when using this search?

A possible more optimized solution that still only relies on the database search capabilities could be to introduce search columns to relevant searchable columns. In the search columns you can then store the normalized text value of the value you want to search. By doing this you don't need to execute several variant clauses and don't need to transform text values on the fly.

[kunyao-cofinity-x]

@kunyao-cofinity-x

A first feedback to the pull request: There is now a lot of additional filters that can be combined with business partner searches. And these filters also include variant search clauses and and transforms like making everything lower case. With these very powerful search I could imagine that the performance might take a drastic hit on large data sets. Did you do a performance test on your data set when using this search?

A possible more optimized solution that still only relies on the database search capabilities could be to introduce search columns to relevant searchable columns. In the search columns, you can then store the normalized text value of the value you want to search. By doing this you don't need to execute several variant clauses and don't need to transform text values on the fly.

Hi @nicoprow, thanks for the feedback. For the performance improvement. I plan to add a few columns in the pool for the value normalization.

• legal_name_normalizing
• phy_street_name_normalizing
• phy_country_normalizing

In those three columns, we would see the German umlaut. We can normalize them in advance so we don't convert them on the fly. Based on this idea, do you have a suggestion?

[nicoprow]

@kunyao-cofinity-x
A first feedback to the pull request: There is now a lot of additional filters that can be combined with business partner searches. And these filters also include variant search clauses and and transforms like making everything lower case. With these very powerful search I could imagine that the performance might take a drastic hit on large data sets. Did you do a performance test on your data set when using this search?
A possible more optimized solution that still only relies on the database search capabilities could be to introduce search columns to relevant searchable columns. In the search columns, you can then store the normalized text value of the value you want to search. By doing this you don't need to execute several variant clauses and don't need to transform text values on the fly.

Hi @nicoprow, thanks for the feedback. For the performance improvement. I plan to add a few columns in the pool for the value normalization.

* legal_name_normalizing

* phy_street_name_normalizing

* phy_country_normalizing

In those two columns, we would see the German umlaut. We can normalize them in advance so we don't convert them on the fly. Based on this idea, do you have a suggestion?

I think this would be a very straight forward approach which does not take so much effort. The most important part is just to create a migration script to fill the new columns for the existing entries and update the column values with each business partner update.

I think it sounds like a good approach without the need of implementing a too complex alternative

[nicoprow]

I left some change requests and comments here. Some of these comments are actually regarding the underlying requirements. We can use those in the next refinement session as basis for debate

[nicoprow]

It says it should search by ISO country code but that would equal our enum value, so why is this a free text field? Or is it also expected to find matches on full country names like 'Germany'?

[nicoprow]

Contributes to #603

[kunyao-cofinity-x]

Two changes have been added

• Data model of search result. Extension of BP search function sig-release#603 (comment)
• Normalized data columss have been added. feat: Implement search logic for business partner extension #1382 (comment)

[kunyao-cofinity-x]

Hello @nicoprow @SujitMBRDI ,

Here is the summary of the commit:

• Search request "id" change to "bpn". It is allowed BPNL and BPNS.
• Search request "street" adapt StreetDto.
• Search parameters change from "only" to "Include", ex, IncludeLegalEntities, IncludeSites, IncludeAdditionalAddresses
• Search logic based on address (LogisticAddressRepository)
• No additional columns added, no additional maintenance efforts for the normalized value.
• A normalized function in the database returns the value during table queries.
• The normalized function applies to legal name, street, and city.
• docs/api/pool.json and pool.yaml have been updated.

[kunyao-cofinity-x]

Summary of the last commit:

• Refactoring the LogisticAddressRepository.searchBusinessPartner logic.
• Refactoring the BusinessPartnerSearchService.searchBusinessPartner logic.
• Fixing the issue when the street is null in the search request.

CC: @nicoprow @SujitMBRDI

[nicoprow]

Core logic looks good and the behaviour of the search is quite mature.

I found some issues and added them in the review comments.

Additionally, a new entry in the CHANGELOG.md is missing for this feature

[nicoprow]

I believe this should be a 400 Error response instead. Otherwise it is not very clear to the API user why no results are being given. I would instead prefer an error response with an appropriate message for each case

[nicoprow]

Something wrong about the pagination logic. If I have no data in my result I get a contentSize of 10 due to the pagination request size attribute.

[kunyao-cofinity-x]

If there is no result in content, the page size value adapts the value from the paginationRequest.size that the value is 10, should it be correct?

[nicoprow]

For pagination the number of total elements is the the total number of objects you can page through. The content size is the number of objects found in the current page and is the same as content.size

[nicoprow]

Unrelated to the feature. Please revert back for more clarity

[kunyao-cofinity-x]

Hi @nicoprow , thank you for the feedback. Here is the summary of the commit:

1. Change street to streetName in the search request. The type is String.
2. Return 400 and an error message when bpn, legalName, and the filter parameter are empty.
3. Mapping alternativePostalAddress in the search result.
4. Create DTOs under the Pool-API for pool search result, remove dependency of bpdm-gate-api
   org.eclipse.tractusx.bpdm.pool.api.model.response.BusinessPartnerSearchResultDto
5. revert org.eclipse.tractusx.bpdm.pool.service.compareStates, logic does not change, only indentation.
6. Update CHANGELOG.md
7. bpn search request support "BPNA"

[nicoprow]

Hi @nicoprow , thank you for the feedback. Here is the summary of the commit:

1. Change street to streetName in the search request. The type is String.

2. Return 400 and an error message when bpn, legalName, and the filter parameter are empty.

3. Mapping alternativePostalAddress in the search result.

4. Create DTOs under the Pool-API for pool search result, remove dependency of bpdm-gate-api
   org.eclipse.tractusx.bpdm.pool.api.model.response.BusinessPartnerSearchResultDto

5. revert org.eclipse.tractusx.bpdm.pool.service.compareStates, logic does not change, only indentation.

6. Update CHANGELOG.md

7. bpn search request support "BPNA"

Thank you for the adaptions. It looks good to me.

There are also other comments that are missing feedback. Will you work on them as well?

Regarding the mapping, the alternative address is now mapped correctly but the physical address still misses some fields (see the DTO).

[kunyao-cofinity-x]

Hi @nicoprow , here is the second wave commit summary

1. Reverted the files that are not related to this PR
2. Add pagination to the LogisticAddressRepository, and remove the additional pagination function.
3. Move validation to the service level
4. Add BusinessPartnerExceptionHandler and BusinessPartnerSearchException to handle the exception.
5. Refactoring the testing cases
6. Add country code validation
7. Mapping missing PhysicalAddress attributes

[nicoprow]

Basically, looks good to me. The only thing I noticed is that the migration script needs to be increased to version 7.3.0, since we had some version increase meanwhile.

Once that is done I think it's ready to merge and I would approve.

[kunyao-cofinity-x]

In the last commit, two changes.

1. Changed the native SQL to JPQL
2. Changed the migration script to V7.3.0

[nicoprow]

Seems like the changes to the Pool actually increased its memory size, I guess because of additional prepared statements?

Anyway, I checked the Charts install with a new Pool memory limit of 1200Mi and it worked fine. You can have a look here:

bpdm/charts/bpdm/charts/bpdm-pool/values.yaml

Line 73 in c39d37e

memory: 1200Mi

bpdm/charts/bpdm/charts/bpdm-pool/values.yaml

Line 76 in c39d37e

memory: 1200Mi

Please change your pull request accordingly and then it should be good to merge