CCI-MOC / flocx-market

2 stars 9 forks source link

Error Raising in dbapi #109

Closed ljmcgann closed 5 years ago

ljmcgann commented 5 years ago

Creates a set of exceptions and modifies the dbapi to raise these errors when appropriate. TG-163

codecov-io commented 5 years ago

Codecov Report

Merging #109 into master will decrease coverage by 0.36%. The diff coverage is 91.41%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #109      +/-   ##
=========================================
- Coverage   93.26%   92.9%   -0.37%     
=========================================
  Files          51      52       +1     
  Lines        2005    2114     +109     
=========================================
+ Hits         1870    1964      +94     
- Misses        135     150      +15
Impacted Files Coverage Δ
flocx_market/tests/unit/api/test_app_bid.py 100% <100%> (ø) :arrow_up:
flocx_market/tests/unit/api/test_app_contract.py 100% <100%> (ø) :arrow_up:
flocx_market/tests/unit/api/test_app_offer.py 100% <100%> (ø) :arrow_up:
...s/unit/api/test_app_offer_contract_relationship.py 100% <100%> (ø) :arrow_up:
...objects/test_object_offer_contract_relationship.py 100% <100%> (ø) :arrow_up:
flocx_market/common/exception.py 100% <100%> (ø)
flocx_market/db/sqlalchemy/api.py 93.93% <80.55%> (-1.55%) :arrow_down:
flocx_market/api/contract.py 91.89% <87.5%> (-4.78%) :arrow_down:
flocx_market/api/bid.py 91.89% <88%> (-4.78%) :arrow_down:
flocx_market/api/offer.py 91.89% <88%> (-4.78%) :arrow_down:
... and 4 more

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 e59892b...e6b648e. Read the comment docs.

ljmcgann commented 5 years ago

Does it really make sense to return a "ResourceNotFound" error when doing a get? Currently the different get_all functions return an empty list if the specific query finds nothing But Filip's new get_alls will accept a query filter. Combining our code will give us a get that returns an error when it does not find an offer by id, but get_all(marketplace_id==value) will return an empty list. These two functions can effectively do the same thing but with inconsistent return types

ljmcgann commented 5 years ago

Perhaps the only error raising should be the no permissions one, and resources not being found should just return None

tzumainn commented 5 years ago

I do think we should throw ResourceNotFound rather than None; if someone is specifically querying for an object based on a UUID, or trying to update it or delete it - then there's some issue going on somewhere if that object can't be found.

This looks generally good! Just needs a rebase based on lars's update.

ljmcgann commented 5 years ago

This code should be merged after Filip merges his changes to the filters, as I will have to update my code anyways and merging will likely break all of his code again

ljmcgann commented 5 years ago

Is there anything holding up this pr at this point. Mainn has approved it but it says merging is blocked for requested changed (that I addressed, I believe)