Skip to content

Conversation

@tmillingtonhic
Copy link
Collaborator

Screenshots (if relevant)

Screenshot 2026-01-05 at 14 50 49
Screen.Recording.2026-01-05.at.15.01.08.mov

Describe your changes

Issue ticket link

https://hdruk.atlassian.net/browse/GAT-8219

Environment / Configuration changes (if applicable)

Requires migrations being run?

If not using the pre-push hook. Confirm tests pass:

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have added appropriate unit tests
  • I have created mocks for unit tests (where appropriate)
  • I have added appropriate Behat tests to confirm AC (if applicable)
  • I have added Swagger annotations for new endpoints (if applicable)
  • I have added audit logs for new operation logic (if applicable)
  • I have added new environment variables to the .env.example file (if applicable)
  • I have added new environment variables to terraform repository (if applicable)

@github-actions
Copy link

github-actions bot commented Jan 5, 2026

🎉 Great job! Your PR title follows the correct format. 🚀

]);

return response()->json([
$collection = $this->getCollectionActiveById($id);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're now streaming the collection data, why do we need the additional memory to store as a variable? Was the original call to the function problematic?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd hazard a guess that we're actually using more memory here, just masking with the stream

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The issue is in returning the json - Laravel uses a large amount of memory constructing the response()->json(). Without calling the response()->json() method it only takes about 30MB to construct the collection. By streaming the json it means we don't have to construct the full json response before returning.

Rewriting $this->getCollectionActiveById to stream the results back would probably reduce memory consumption further but just streaming the json was sufficient for this ticket.

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.

4 participants