Skip to content
This repository has been archived by the owner on Sep 28, 2021. It is now read-only.

clarify unimplemented RequestContext metadata #181

Closed
wants to merge 1 commit into from

Conversation

pettermahlen
Copy link
Member

Previously, if users upgrading do not notice that there is a new
RequestContext.metadata() method, their code will start failing
in strange ways (requests timing out was the concrete example)
because of the values supplied by the default implementation. It's
a better solution to throw an exception, forcing implementations
to actually override this method.

Previously, if users upgrading do not notice that there is a new
RequestContext.metadata() method, their code will start failing
in strange ways (requests timing out was the concrete example)
because of the values supplied by the default implementation. It's
a better solution to throw an exception, forcing implementations
to actually override this method.
@codecov-io
Copy link

Current coverage is 69.88% (diff: 0.00%)

Merging #181 into master will increase coverage by 0.07%

@@             master       #181   diff @@
==========================================
  Files           144        144          
  Lines          2952       2949     -3   
  Methods           0          0          
  Messages          0          0          
  Branches        239        239          
==========================================
  Hits           2061       2061          
+ Misses          845        842     -3   
  Partials         46         46          

Powered by Codecov. Last update 3e2a70f...da46f22

@udoprog
Copy link
Contributor

udoprog commented Nov 16, 2016

This caught my attention and I wanted to investigate what would happen if you'd just remove the defender method completely. It opened my eye to another compatibility concern regarding defender methods and I ended up doing some experiments to verify my assumptions.

While this is part of the public API, consumers won't notice as long as an implementation is present. Assuming all implementations have overridden it so far by convention, a mismatch in API version (more recent than impl) would also be safe. default ended up not being part of the method signature so that method dispatch will work as before for binary compatibility whether it is present or not. What would happen is if a previous implementation does not override the method and the API is updated independently, an AbstractMethodError would be raised. Which is arguably worse than a RuntimeException.

@pettermahlen
Copy link
Member Author

@udoprog that's sort of what I was thinking, too, when I realised that this default implementation really didn't help users, as it was broken. So probably, this change (adding metadata and giving that meaning in implementation) should really have been part of a major version upgrade, I guess.

The current implementation isn't really any better, I think, than getting a runtime error due to a missing method. In either case, you need to trigger that code path in order to detect the problem, instead of getting help from the compiler. My bad, but at least I learned something, I hope.

@pettermahlen
Copy link
Member Author

And I didn't notice until now that this PR is for master, which is anyway backwards-incompatible. So rather than replicating what was done in #180 for 1.x, for v2, we should just remove the default implementation.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants