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

fix/multiple.includes.nestedResources #373

Merged

Conversation

Martinarbez
Copy link
Contributor

This Pull Request introduces several changes to address a key issue and improve the functionality and testing of the application:

Problem Solved: The application was previously unable to handle queries for a resource with multiple nested resources simultaneously. If multiple nested resources were included in the query, only the last one was returned. This issue has been resolved with the changes in this PR.

  1. Enhancement in knex-processor.ts: The parseOperationIncludedRelationships function has been updated to ensure that the acumRelationships object correctly accumulates relationships without overwriting existing ones. This is achieved by spreading the existing relationships before adding a new one. This change is part of the solution to the problem described above.

  2. Refactoring in serializer.ts: The serialization process has been simplified. Previously, the code was creating a new array with unique type_id combinations from includedData and then mapping over this array to find the corresponding items in includedData. This has been replaced with a direct return of includedData, which simplifies the code and may improve performance.

  3. Addition of a new test in nestedResources.test.ts: A new test has been added to verify the correct retrieval of the first article's author's votes and comments. This test will help ensure that the application's data retrieval and serialization processes are working as expected, and that multiple nested resources can be retrieved simultaneously.

@Martinarbez Martinarbez added the bug Something isn't working label Dec 22, 2023
@Martinarbez Martinarbez self-assigned this Dec 22, 2023
Copy link
Member

@joelalejandro joelalejandro left a comment

Choose a reason for hiding this comment

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

Excellent work!

@marcemira marcemira merged commit cbd5337 into kurierjs:main Dec 22, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants