Skip to content

Implemented fallback mechanism to TRAX v2 endpoints that obtain data…#394

Open
JavaDeveloper456788 wants to merge 1 commit into
mainfrom
fix/GRAD2-3033
Open

Implemented fallback mechanism to TRAX v2 endpoints that obtain data…#394
JavaDeveloper456788 wants to merge 1 commit into
mainfrom
fix/GRAD2-3033

Conversation

@JavaDeveloper456788

Copy link
Copy Markdown

… from redis cache for school and district service, added unit tests, added new endpoints

…from redis cache for school and district service, added unit tests, added new endpoints
@sonarqubecloud

sonarqubecloud Bot commented Jan 7, 2025

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
66.3% Coverage on New Code (required ≥ 80%)
B Maintainability Rating on New Code (required ≥ A)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

log.debug("Get Schools from Redis Cache returned empty");
List<School> schools = getSchoolsFromInstituteApi();
if ((schools != null) && (!schools.isEmpty())) {
loadSchoolsIntoRedisCache(schools);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove code block here. Cache is initialized on load by a single pod using a cache key. The only reason the cache returns empty is the edge case when one pod is loading cache while another gets the request at the controller. Just return the schools List from Institute and we should be good.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed, created a new PR #395

* @throws JsonProcessingException the json process exception
*
*/
public static HashMap<String, String> searchStringsToHTTPParams(HashMap<String, String> searchStringMap) throws JsonProcessingException {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Consume and return an interface (Map) instead of an implementation

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed, created a new PR #395

HashMap<String, String> searchInput = new HashMap<>();
searchInput.put(key, value);

try {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nested catch block is unnecessary.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed, created a new PR #395

List <DistrictEntity> districtEntities = this.restService.get(constants.getDistrictsPaginated(),params,
List.class, webClient);
return districtTransformer.transformToDTO(districtEntities);
} catch (WebClientResponseException e) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Exception and WebClientException are caught in the RESTService.get method already which throws a ServiceException. Either catch the service exception and deal with it here or pass it up to the controller level where it can return an error to the caller.

Also, be aware that you can utilize the RestErrorHandler class to capture thrown errors and deal with them at the controller level

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed, created a new PR #395

public List<District> getDistrictsFromRedisCache() {
log.debug("**** Getting districts from Redis Cache.");
return districtTransformer.transformToDTO(districtRedisRepository.findAll());
Iterable<DistrictEntity> districtEntities= districtRedisRepository.findAll();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove this. We don't want to trigger a wholesale refresh of the cache. We are only interested in specific districts, not for get all.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed, created a new PR #395

if ( districtEntity == null){
log.debug("Getting district by district no from Redis Cache returned empty");
List<District> districts = this.getDistrictsBySearchCriteriaFromInstituteApi("districtNumber",districtNumber );
if ((districts != null) &&(!districts.isEmpty())){

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See comment on getDistrictByIdFromRedisCache method regarding null checks on Optional

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed created a new PR #395

return districts;
}

public District getDistrictByIdFromInstituteApi(String districtId) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Again no need to catch Exceptions here. You can throw a ServiceException like the updateDistrictCache method below and also expect a District object returned instead of an Optional.

@JavaDeveloper456788 JavaDeveloper456788 Jan 14, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed created a new PR #395. The method already returns District object

if (schoolEntity == null) {
log.debug("Get School by Mincode from Redis Cache returned null");
List<School> schools = getSchoolsBySearchCriteriaFromInstituteApi("mincode", mincode);
if ((schools != null) && (!schools.isEmpty())) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See comment on DistrictService.getDistrictByIdFromRedisCache method regarding null checks on Optional

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed created a new PR #395




public List <School> getSchoolsBySearchCriteriaFromInstituteApi(String key, String value) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See comments on getDistrictsBySearchCriteriaFromInstituteApi. Please change accordingly.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed created a new PR #395

public List<SchoolDetail> getSchoolDetailsFromRedisCache() {
log.debug("**** Getting school Details from Redis Cache.");
return schoolDetailTransformer.transformToDTO(schoolDetailRedisRepository.findAll());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove this code block. We don't want to trigger a wholesale refresh of cache at this point. We are only interested in calling back to institute for individual institutes.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed created a new PR #395

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.

3 participants