blaggacao commented 5 years ago

It commonly occurs that policy resolution does not involve http, eg. in case of local caching. This commit introduces a hook to implement custom resolvers. It also polishes naming as required by this implementation.

blaggacao commented 5 years ago

@etobella Also a policy cached policy property would be interesting:

If i have to sign many documents in a row, I can declare the policy object globally and use spare the time for policy fetching and signature calculation having it stored in an object cache. Would you agree / accept a PR?

blaggacao commented 5 years ago

Note: removal of the PolicyId class was indicated as it's only method accessed an undefined property (self.identifier) indicating the normalized form would be moving that method to the special implementation of PolicyIdclass: the GenericPolicyId class. (despite the naming which suggested an inverse relationship)

blaggacao commented 5 years ago

@etobella Test failure is due to a failed URL handshake when trying to query the policy, therefore unrelated to those changes.

etobella commented 5 years ago

@blaggacao Tests have been fixed. I relaunched travis in order to check that everything is right

etobella commented 5 years ago

You need to rebase in order to check that everything is fine. Could you please?

blaggacao commented 5 years ago

@etobella Ok, thank you so much! Let's see how it goes. I included a couple of other fixes/improvements as I'm using my fork now for development purpose, If you don't agree with anything, please kindly ask me to extract a commit into a separate PR.

codecov-io commented 5 years ago

Codecov Report

Merging #4 into master will decrease coverage by 3.02%. The diff coverage is 81.15%.

@@            Coverage Diff             @@
##           master       #4      +/-   ##
==========================================
- Coverage   95.81%   92.78%   -3.03%     
==========================================
  Files           8        8              
  Lines         311      319       +8     
  Branches       46       44       -2     
==========================================
- Hits          298      296       -2     
- Misses          7       15       +8     
- Partials        6        8       +2

Impacted Files	Coverage Δ
src/xades/xades_context.py	`97.1% <100%> (ø)`	:arrow_up:
src/xades/policy.py	`84.55% <80%> (-7.63%)`	:arrow_down:

blaggacao commented 5 years ago

@etobella

As for codecov it looks like the unspecific implementation of validate_policy_node which does not depend on initialized value (no policy cache) would not be tested with the current adaptions.

I construed a use case of something like:

If I don't know the chosen implementation I can use the Policy class to verify it anyways (though not optimized).
However, this is only a vague idea of why this was implemented on the generic level. Maybe there is no use case for such.
In the latter case, it would be more correct to just define the interface of this method in the Policy class.

How should I adapt?

etobella commented 5 years ago

Remove it from policy (only a pass) and keep it on the Generic, I think it is the best approach.

By the way, I didn't know the usage of ElementMaker, really interesting :smile:

blaggacao commented 5 years ago

@etobella Thanks for your input. Please see the latest commit, I chose NotImplementedError instead. Also naming the Policy class BasePolicy better reveals it's character and role. I hope you're fine with it.

etobella commented 5 years ago

Merging it, I will not create a new version until we finish the issues you commented #7 #6

blaggacao commented 5 years ago

Perfect, I'll retake shortly, had another priority issue coming in...

etobella / python-xades

Add policy resolver hook #4

Codecov Report