Skip to content

Fixed including extra resources from OnApplyIncudes #1011

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

Merged
merged 4 commits into from
Jun 14, 2021
Merged

Conversation

bart-degreed
Copy link
Contributor

@bart-degreed bart-degreed commented Jun 10, 2021

Makes the serializer aware of changes to inclusion trees applied from resource definitions. This enables adding extra includes from a resource definition.

Additional notes:

  • Refactored existing tests to use friendlier models.
  • Makes ResponseResourceObjectBuilder a bit faster, because it no longer evaluates all constraints on each include.
  • Improved hit tracking assertions for resource definition callbacks.
  • Optimization: removed callback invocations for to-one include (see QueryLayerComposer) because their results are never used.

I recommend reviewing the commits one by one.

Fixes #989.

QUALITY CHECKLIST

  • Changes implemented in code
  • Complies with our contributing guidelines
  • Adapted tests
  • N/A: Documentation updated
  • N/A: Created issue to update Templates: {ISSUE_NUMBER}

@bart-degreed bart-degreed requested a review from maurei June 10, 2021 17:32
@bart-degreed bart-degreed changed the title Extra includes Fixed including extra resources from OnApplyIncudes Jun 11, 2021
@codecov
Copy link

codecov bot commented Jun 11, 2021

Codecov Report

Merging #1011 (ed91e1c) into master (f145564) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1011      +/-   ##
==========================================
+ Coverage   91.47%   91.49%   +0.01%     
==========================================
  Files         274      275       +1     
  Lines        7652     7666      +14     
==========================================
+ Hits         7000     7014      +14     
  Misses        652      652              
Impacted Files Coverage Δ
...NetCore/Configuration/JsonApiApplicationBuilder.cs 100.00% <100.00%> (ø)
...tCore/Queries/Expressions/IncludeChainConverter.cs 100.00% <100.00%> (ø)
...tNetCore/Queries/Internal/EvaluatedIncludeCache.cs 100.00% <100.00%> (ø)
...iDotNetCore/Queries/Internal/QueryLayerComposer.cs 97.54% <100.00%> (+0.06%) ⬆️
...rnal/ResourceDefinitionQueryableParameterReader.cs 100.00% <100.00%> (ø)
...iDotNetCore/Resources/JsonApiResourceDefinition.cs 100.00% <100.00%> (ø)
...erialization/AtomicOperationsResponseSerializer.cs 98.24% <100.00%> (+0.09%) ⬆️
...lization/Building/ResponseResourceObjectBuilder.cs 91.30% <100.00%> (-0.54%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f145564...ed91e1c. Read the comment docs.

Bart Koelman added 4 commits June 11, 2021 19:58
…anges` into account. This enables adding extra includes from a resource definition.

This commit makes `ResponseResourceObjectBuilder` a bit faster too, because it no longer evaluates all constraints on each include.
Optimization: Removed callback invocations for to-one include (see QueryLayerComposer) because their results are never used.
@maurei maurei merged commit 67e0739 into master Jun 14, 2021
@maurei maurei deleted the extra-includes branch June 14, 2021 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Adding extra includes from IResourceDefinition.OnApplyIncludes() does not work
2 participants