FINERACT-2736: Remove unused FineractEntityAccess (createEntityAccess/addNewEntityAccess dead since 2014/2016, no callers, no test coverage) - #6217
Conversation
5d34633 to
3f96ca4
Compare
|
@adamsaghy please trigger checks whenever possible, thanks a lot ! :) |
galovics
left a comment
There was a problem hiding this comment.
The PR title says "no callers, no test coverage" but I checked on develop before trusting that - this breaks the build.
// FineractEntityAccessUtil.java (NOT touched by this PR, package org.apache.fineract.infrastructure.entityaccess.service)
private final FineractEntityAccessWriteService fineractEntityAccessWriteService; // <- deleted by this PR
private final FineractEntityAccessReadService fineractEntityAccessReadService;FineractEntityAccessUtil is a live @Service with FineractEntityAccessWriteService as a required constructor param, and it's actively wired into SavingsProductWritePlatformServiceJpaRepositoryImpl, ChargeWritePlatformServiceJpaRepositoryImpl, LoanProductWritePlatformServiceJpaRepositoryImpl, and LoanApplicationValidator - this is the "office specific products" access-control feature (the thing behind office-specific-products-enabled). Deleting FineractEntityAccessWriteService breaks FineractEntityAccessUtil's constructor - this doesn't compile, or at minimum fails Spring context startup.
The two-method claim in the title ("createEntityAccess/addNewEntityAccess dead since 2014/2016") might be true for those specific methods, but the PR deletes the whole interface/impl, not just the dead methods. Can we keep FineractEntityAccessWriteService/FineractEntityAccessReadService and only remove the genuinely dead methods (and the table drop needs the same scrutiny)?
|
Hi @galovics , thanks for taking a close look at this, but I think the diff is being misread here.
I also checked whether That also lines up with the CI results on this PR. Let me know if I'm missing something on your end though, always good to double check before merging something like this. |
…/addNewEntityAccess dead since 2014/2016, no callers, no test coverage) createEntityAccess has been an empty stub returning null since the file was first created in 9d11434 (Dec 6 2014). addNewEntityAccess had one caller until 66f8d7c (June 9 2016) rewrote it to bypass the write service, leaving it with zero callers since. Removes FineractEntityAccess, its repository, repository wrapper, data class, constants, and configuration exception, along with the two dead methods and the m_entity_to_entity_access table. Leaves createEntityToEntityMapping/updateEntityToEntityMapping/deleteEntityToEntityMapping and the FineractEntityAccessUtil office-to-product access path untouched. Verified via full-repo grep, compileJava/compileTestJava, scoped spotless check, and Liquibase changeset well-formedness. No existing test coverage for this code, so none removed or added.
3f96ca4 to
3162902
Compare
Description
createEntityAccess(JsonCommand command)has been an empty stub returning null since the file was first created in9d11434ee7(Dec 6 2014). There is no API route,CommandWrapperBuildermethod, permission record, or command handler for it anywhere in the codebase, and there never has been.addNewEntityAccess(...)was implemented from the start and did have one caller,FineractEntityAccessUtil.checkConfigurationAndAddProductResrictionsForUserOffice(...). That changed in66f8d7ce43(June 9 2016), which rewrote the caller to callfineractEntityToEntityMappingRepository.save(...)directly instead, bypassing the write service entirely. Since thenaddNewEntityAccesshas had zero callers.Alongside these two methods, the underlying
FineractEntityAccessdomain object, its repository, its repository wrapper, and them_entity_to_entity_accesstable are only ever referenced from inside these two dead methods. Nothing else touches them.The live access control path,
FineractEntityAccessUtil, used by Charges, Loan Products, and Savings Products, reads fromm_entity_to_entity_mappingviaFineractEntityAccessReadServiceImpl. This is a separate table fromm_entity_to_entity_access.createEntityToEntityMapping,updateEntityToEntityMapping, anddeleteEntityToEntityMappingstay exactly as they are and are not touched by this change.m_entity_to_entity_accesshas 0 rows inload_sample_data.sqland the project's generated schema docs list it at 0 rows as well.Changes made
FineractEntityAccess.java(domain)FineractEntityAccessRepository.javaFineractEntityAccessRepositoryWrapper.javaFineractEntityAccessData.javaFineractEntityAccessConstants.javaFineractEntityAccessConfigurationException.javacreateEntityAccessandaddNewEntityAccessfromFineractEntityAccessWriteService.javaandFineractEntityAccessWriteServiceImpl.java, along with the now-unusedentityAccessRepositoryfield, constructor param, and dead importsupdateEntityAccess/removeEntityAccessstubs from the interface0243_remove_unused_entity_access.xml, a Liquibase changeset that dropsm_entity_to_entity_accessm_entity_to_entity_accesstable definition and sample data block frombarebones_db.sqlandload_sample_data.sqlVerification
.java,.xml,.sqlforFineractEntityAccess,FineractEntityAccessRepository,FineractEntityAccessRepositoryWrapper,FineractEntityAccessData,FineractEntityAccessConfigurationException, andFineractEntityAccessConstants. No stragglers, all hits excluded as still-live (FineractEntityAccessType,FineractEntityAccessNotFoundException,FineractEntityAccessUtil,FineractEntityAccessReadService)fineract-clientorfineract-client-feign, no OpenAPI/swagger entry forcreateEntityAccessDiscussed on the dev mailing list first:
[DISCUSS] ENTITYACCESS | createEntityAccess never implemented since 2014Ticket: FINERACT-2736