search

spotify / heroic

The Heroic Time Series Database
https://spotify.github.io/heroic/
Apache License 2.0
848 stars 109 forks source link

improved Mandatory ID Filter to return info upon rejection #751

Closed sming closed 3 years ago

sming commented 3 years ago

Note for reviewers

Checkout out the Template Method design pattern before reviewing this PR 👍🏻

codecov[bot] commented 3 years ago

Codecov Report

Merging #751 (1ec4570) into master (7a830df) will increase coverage by 0.11%. The diff coverage is 75.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #751      +/-   ##
============================================
+ Coverage     55.02%   55.13%   +0.11%     
- Complexity     3153     3162       +9     
============================================
  Files           746      748       +2     
  Lines         20389    20395       +6     
  Branches       1339     1339              
============================================
+ Hits          11219    11245      +26     
+ Misses         8682     8662      -20     
  Partials        488      488
Impacted Files Coverage Δ Complexity Δ
.../main/java/com/spotify/heroic/http/HttpServer.java 16.52% <0.00%> (ø) 3.00 <0.00> (ø)
...ava/com/spotify/heroic/servlet/ShutdownFilter.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
.../java/com/spotify/heroic/servlet/SimpleFilter.java 88.88% <88.88%> (ø) 4.00 <4.00> (?)
...potify/heroic/servlet/MandatoryClientIdFilter.java 100.00% <100.00%> (+20.00%) 4.00 <2.00> (-1.00) :arrow_up:
...otify/heroic/ws/MandatoryClientIdErrorMessage.java 100.00% <100.00%> (ø) 2.00 <2.00> (?)
.../main/java/com/spotify/heroic/ws/ErrorMessage.java 57.14% <0.00%> (+57.14%) 4.00% <0.00%> (+4.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 7a830df...1ec4570. Read the comment docs.

lmuhlha commented 3 years ago

One question I had, why the switch from 400 to 503?

sming commented 3 years ago

One question I had, why the switch from 400 to 503? that is a very good question, it should be 400. 503 is wrong. Weirdly though IIRC there's a test proving that it returns 400...

sming commented 3 years ago

Hey @lmuhlha , @samfadrigalan could one of you please approve - I addressed all the issues raised. Cheers.

adsail commented 3 years ago

@sming One quick thought -- It might be a good idea to update the public docs for /query/metrics to stipulate the header requirement. It's currently part of the sample cURL but not called out as a requirement.

https://github.com/spotify/heroic/blob/master/docs/content/_endpoints/post-query-metrics.md