Skip to content

Fix exception propagation #1021

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 7 commits into from
Jun 23, 2021
Merged

Fix exception propagation #1021

merged 7 commits into from
Jun 23, 2021

Conversation

bart-degreed
Copy link
Contributor

Fixes #1017.

@bart-degreed bart-degreed marked this pull request as draft June 22, 2021 17:13
Bart Koelman and others added 4 commits June 22, 2021 22:56
…ed while disposing PlaceholderResourceCollector that threw too, resulting in a 500 error response. And it produced noisy logs in cibuild, coming from ExceptionHandler. This commit swallows the second exception so the original one can propagate through the pipeline as intended.
…e that not specifying it results in EF Core configuring this relationship using an identifying FK on the dependent side (Shipment), which is a type of modeling that does not make sense in the scope of JSON:API.
@bart-degreed bart-degreed force-pushed the fix-exception-propagation branch from 320eab6 to 451bb32 Compare June 22, 2021 22:29
Bart Koelman added 2 commits June 23, 2021 00:32
…d to use an explicit foreign key accordingly. It turns out that not in all cases this makes a difference. I wonder under which circumstances EF Core chooses to use an identifying foreign key and when it does not.
@bart-degreed bart-degreed force-pushed the fix-exception-propagation branch from 8a68a7a to 77be4ff Compare June 22, 2021 22:32
@bart-degreed bart-degreed marked this pull request as ready for review June 22, 2021 22:34
@bart-degreed
Copy link
Contributor Author

bart-degreed commented Jun 22, 2021

I considered adding a note to our documentation on HasOne, but it's not entirely clear to me when EF Core chooses to create an identifying foreign key. It does not always do that (see commit details). One condition is that the primary keys of both resources must be of the same type, which usually is not the case in our examples.

@bart-degreed bart-degreed requested a review from maurei June 22, 2021 22:35
@codecov
Copy link

codecov bot commented Jun 23, 2021

Codecov Report

Merging #1021 (c38d223) into master (ceedaa8) will decrease coverage by 0.02%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1021      +/-   ##
==========================================
- Coverage   91.49%   91.47%   -0.03%     
==========================================
  Files         275      275              
  Lines        7668     7671       +3     
==========================================
+ Hits         7016     7017       +1     
- Misses        652      654       +2     
Impacted Files Coverage Δ
...tCore/Repositories/PlaceholderResourceCollector.cs 91.30% <50.00%> (-8.70%) ⬇️
...Core/Repositories/EntityFrameworkCoreRepository.cs 99.20% <100.00%> (ø)

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 ceedaa8...c38d223. Read the comment docs.

@maurei maurei merged commit e42a7a4 into master Jun 23, 2021
@maurei maurei deleted the fix-exception-propagation branch June 23, 2021 09:06
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.

Consecutive exception hides original error
2 participants