[NT-0] fix: Removing an asset collection assert #841
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, we invoked an assert when encountering an asset collection type not known to CSP. Similar to what we're doing elsewhere, this change removes the assertion in favor of an error log message instead.
Roughed out test for validating asset collection DTO parsing.
A new set of test permutations to validate CSP parses DTO types to expected CSP types, and that parsing invalid DTO types returns the default result.
CSP assumed certain fields would always be present on prototype DTOs. Not unreasonable, but also a source of uncaught exceptions if they weren't. Fixed by following the same pattern as with other fields, and only setting the properties following a check to see if the DTO has them. Also, removed the 'Charity' DTO type, as this was agreed in a recent CSP working group session.
🚀 Pull Request Review GuidelinesThank you for taking the time to review this Pull Request. The following is a summary of our Pull Request guidelines. The full guidelines can be found here. 💬 How to Provide FeedbackWe use a comment ladder when leaving review comments to avoid any ambiguity.
All comments should be prefixed with one of the above tags, for example: 🎯 PR Author Focus Areas
Thanks again for taking the time to review this Pull Request. |
MAG-AdamThorn
requested changes
Oct 27, 2025
Validating that in the case of an unknown asset type, CSP emits the expected log message.
MAG-AdamThorn
approved these changes
Oct 29, 2025
MAG-mv
approved these changes
Oct 29, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The Issue
A CSP client app developer recently reported a crash when attempting to enter a space. Turns out it was due to there being an asset collection/prototype in the space that did not conform to the types CSP expects, which caused CSP to fire an assert.
The Fix
To fix, we follow the same pattern as with Asset.cpp and emit an
Errorlog message instead of an assert (see here).The Tests
Initial test implementation to validate type parsing for the DTO came in here. Then refined that to use the test matrix approach here.
Bonus
The tests highlighted that CSP was (not unreasonanbly) assuming certain fields would always exist on prototype DTOs. Problem was if they didn't exist, it would lead to an uncaught exception. To fix, we now follow the same pattern for all prototype DTO fields, first testing for then existence, and only then retrieving their value.
Added bonus, whilst in the area I elected to nuke the
CharityDTO type CSP was checking for, as it was agreed we should remove it in a recent CSP working group session.Both bonus changes can be seen here.