On Mon, Jun 20, 2011 at 3:42 AM, Simon Laws <[email protected]> wrote: >> >> Simon, >> >> Maybe I'm not understanding you, but I think this is working as it >> should. We are forced to use a two step process, but the policies do >> get removed from the EPs/EPRs. >> >> On the EP side, we loop through the list of component services and see >> if we need to remove any policy sets. If we do, we also remove the >> same policy sets from the endpoints for the service (since the >> endpoints will have inherited the policy sets earlier in the build >> process.) >> >> I think we're kind of stuck with this as it is -- the appliesTo >> processing can not directly apply to the EP/EPR because it is XPath >> based and needs to work against the actual composite model. We also >> can't move it to occur prior to inheritance because it is legal to >> specify a policy set on an element where it does not apply and have it >> be inherited by an element where it does apply. >> >> Brent >> > > Hi Brent > > I agree both that we have to run the appliesTo processing after the > inheritance processing and that the XPath has to be applied against > the composite model and not endpoints. I note that there were some > recent changes to the code but looking at the current code for the > part that deals with the EP > > 1 for (ComponentService componentService : > component.getServices()) { > 2 List<PolicySet> policySetsToRemove = > checkAppliesToSubject(document, appliesToSubjects, topComposite, > componentService, componentService.getPolicySets()); > 3 for (Endpoint ep : componentService.getEndpoints()) { > 4 ep.getPolicySets().removeAll(policySetsToRemove); > 5 if (ep.getBinding() instanceof PolicySubject) > { > 6 List<PolicySet> bindingPolicySetsToRemove > = > checkAppliesToSubject(document, appliesToSubjects, topComposite, > (PolicySubject)ep.getBinding(), > ((PolicySubject)ep.getBinding()).getPolicySets()); > 7 > ep.getPolicySets().removeAll(bindingPolicySetsToRemove); > 8 } > 9 } > 10 } > > A) during the policy processing all of the policies that are relevant > to the EP, regardless of where they are attached, should be > accumulated on the endpoint by this stage so I don't see the need to > check both the service and the binding. The only thing we need to > check is the binding as the applies to will, I believe, only refer to > binding or implementation types. > > B) passing in ((PolicySubject)ep.getBinding()).getPolicySets()) as the > set of policy sets to check is not correct I believe because it will > not hold the full set of policy sets aggregated at the EP > > C) why is ep.getPolicySets().removeAll(policySetsToRemove); called > inside the loop. From A) and B) I don't think this line is required at > all now. > > So I think we can reduce the above to > > for (ComponentService componentService : > component.getServices()) { > for (Endpoint ep : componentService.getEndpoints()) { > if (ep.getBinding() instanceof PolicySubject) { > List<PolicySet> policySetsToRemove = > checkAppliesToSubject(document, appliesToSubjects, topComposite, > (PolicySubject)ep.getBinding(), ep.getPolicySets()); > > ep.getPolicySets().removeAll(policySetsToRemove); > } > } > } > > Note. I still check against the binding model but get the policy sets > from the EP. Similar changes would apply to reference also. > > With the code as it currently is in svn I'm getting failures in the > policy compliance tests 9006, 9009, 9019, 9020, 9021. For 9006 it was > expecting an exception and the test is now running successfully. Not > sure which changes it's related to so I'll look at bit more closely as > I've been changing policy stuff as well. > > Regards > > Simon > > -- > Apache Tuscany committer: tuscany.apache.org > Co-author of a book about Tuscany and SCA: tuscanyinaction.com >
A) I disagree that we only have to check the binding. The appliesTo XPath expression could potentially point to any element in the composite. Also, consider the case where an intent is specified on a higher level element: <service requires="myIntent"> <binding.ws> </service> "myIntent" could potentially not apply here, but we wouldn't be checking it if we only check the binding. B) The binding policy sets will not encompass the full set of policy sets inherited by the endpoint, but the idea is that we will check every element that the endpoint has inherited from (I don't think this is correct today, as the component is missing.) C) It's called from within the loop because "policySetsToRemove" is the set of policy sets on the service which do not apply to the endpoint, and it needs to be removed from each endpoint underneath the service. The code you listed I think would be correct if we assume that appliesTo can only reference a binding element (and implementations, though we don't care about that here.) Maybe we should be doing this (it would make sense), but without that limit in the spec I don't think we can assume it. I'll take a look at the CTS failures.. I wouldn't expect them to be related to the changes to the appliesTo builder because those should only affect services with multiple endpoints, but there have certainly been some other changes that could break them. Brent
