Re: Review Request 32329: Extract job key from RPC parameters
On March 23, 2015, 8:59 p.m., Joshua Cohen wrote:
Thanks, this is already much easier to follow.
One general question on the overall approach: do you think the DRY benefits
of using composed `StructFieldGetter`s to generate the functions that allow
walking from the starting type to `JobKey` outweigh the readability
improvements of using a fully explicit mapping? I.e.
FunctionJobUpdateRequest, JobKey UPDATE_REQUEST_TO_JOB_KEY = new
Function... {
@Override
public JobKey apply(JobUpdateRequest request) {
return request.getTaskConfig().getJobKey();
}
};
Bill Farner wrote:
I raised this question offline as well. It's not clear to me that this
DRY-ness is worth the complexity. Required null checking in the call chain
is one downside to the more direct approach you illustrated.
Yeah, Kevin and I discussed offline and he mentioned the null checking. If
that's really the only benefit, would it make sense to simply catch the NPE and
return Optional.absent?
- Joshua
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32329/#review77289
---
On March 23, 2015, 7:14 p.m., Kevin Sweeney wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32329/
---
(Updated March 23, 2015, 7:14 p.m.)
Review request for Aurora, Joshua Cohen and Bill Farner.
Bugs: AURORA-1187
https://issues.apache.org/jira/browse/AURORA-1187
Repository: aurora
Description
---
Apologies for the large diff, this wound up needing to input validation at
the AOP layer.
Probably the best place to start reading this diff is ApiSecurityIT to see
the feature this patch enables.
Diffs
-
config/pmd/custom.xml 521fd500146eb2e45f8e77c5c3c0cce330fedabb
src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java
827e85b6cac8bd52359610bbc2002973a769705c
src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java
2408cd1f9af5f109a339f5c78134465cb117f7fc
src/main/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityModule.java
cc9cfd38239f909b8a77bd1a773e31ec30130d41
src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
808987939b2c4a850e488dc033b50b0178e95ba0
src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthenticatingThriftInterceptor.java
PRE-CREATION
src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java
PRE-CREATION
src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroThriftInterceptor.java
4e341e05c34b1be38f0040c26b671a0cc797a771
src/main/java/org/apache/aurora/scheduler/http/api/security/StructFieldGetter.java
PRE-CREATION
src/main/java/org/apache/aurora/scheduler/http/api/security/StructGetter.java
PRE-CREATION
src/main/java/org/apache/aurora/scheduler/http/api/security/StructGetters.java
PRE-CREATION
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
5588d1793d6713ee4581ac9f938d9a8689acb315
src/main/java/org/apache/aurora/scheduler/thrift/aop/AopModule.java
bdd2185f3a7a94b39bcec3c73455e970d87f0c6a
src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java
cafd10f6b705568588c1b92644b482003242fe2e
src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java
ed284f46ac8f01bd6d9e317f995f16d6e666a68d
src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java
76cb691e6d7d4fada3a18fde73aceed7039bcaa4
src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthenticatingThriftInterceptorTest.java
PRE-CREATION
src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java
PRE-CREATION
src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroThriftInterceptorTest.java
d2ba2730c4509dc9a636fd32e9244b0d7fa2884f
src/test/java/org/apache/aurora/scheduler/http/api/security/StructFieldGetterTest.java
PRE-CREATION
src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java
1f24e7d47e1f777ffef19a73d01171fcacd31cdb
src/test/java/org/apache/aurora/scheduler/thrift/aop/AopModuleTest.java
d20c9da3c4944ec8c50fe8d48b7e459ff1c7082b
Diff: https://reviews.apache.org/r/32329/diff/
Testing
---
./gradlew -Pq build
Thanks,
Kevin Sweeney
Re: Review Request 32329: Extract job key from RPC parameters
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32329/ --- (Updated March 23, 2015, 12:14 p.m.) Review request for Aurora, Joshua Cohen and Bill Farner. Bugs: AURORA-1187 https://issues.apache.org/jira/browse/AURORA-1187 Repository: aurora Description (updated) --- Apologies for the large diff, this wound up needing to input validation at the AOP layer. Probably the best place to start reading this diff is ApiSecurityIT to see the feature this patch enables. Diffs - config/pmd/custom.xml 521fd500146eb2e45f8e77c5c3c0cce330fedabb src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 827e85b6cac8bd52359610bbc2002973a769705c src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 2408cd1f9af5f109a339f5c78134465cb117f7fc src/main/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityModule.java cc9cfd38239f909b8a77bd1a773e31ec30130d41 src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java 808987939b2c4a850e488dc033b50b0178e95ba0 src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthenticatingThriftInterceptor.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroThriftInterceptor.java 4e341e05c34b1be38f0040c26b671a0cc797a771 src/main/java/org/apache/aurora/scheduler/http/api/security/StructFieldGetter.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/StructGetter.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/StructGetters.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 5588d1793d6713ee4581ac9f938d9a8689acb315 src/main/java/org/apache/aurora/scheduler/thrift/aop/AopModule.java bdd2185f3a7a94b39bcec3c73455e970d87f0c6a src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java cafd10f6b705568588c1b92644b482003242fe2e src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java ed284f46ac8f01bd6d9e317f995f16d6e666a68d src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java 76cb691e6d7d4fada3a18fde73aceed7039bcaa4 src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthenticatingThriftInterceptorTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroThriftInterceptorTest.java d2ba2730c4509dc9a636fd32e9244b0d7fa2884f src/test/java/org/apache/aurora/scheduler/http/api/security/StructFieldGetterTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java 1f24e7d47e1f777ffef19a73d01171fcacd31cdb src/test/java/org/apache/aurora/scheduler/thrift/aop/AopModuleTest.java d20c9da3c4944ec8c50fe8d48b7e459ff1c7082b Diff: https://reviews.apache.org/r/32329/diff/ Testing --- ./gradlew -Pq build Thanks, Kevin Sweeney
Re: Review Request 32329: Extract job key from RPC parameters
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32329/#review77289
---
Thanks, this is already much easier to follow.
One general question on the overall approach: do you think the DRY benefits of
using composed `StructFieldGetter`s to generate the functions that allow
walking from the starting type to `JobKey` outweigh the readability
improvements of using a fully explicit mapping? I.e.
FunctionJobUpdateRequest, JobKey UPDATE_REQUEST_TO_JOB_KEY = new
Function... {
@Override
public JobKey apply(JobUpdateRequest request) {
return request.getTaskConfig().getJobKey();
}
};
src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
https://reviews.apache.org/r/32329/#comment125179
nit: can re-wrap this after removing the package.
src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java
https://reviews.apache.org/r/32329/#comment125170
should this be `DEPLOY_SERVICE.getUserName()` (same applies above if so).
src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java
https://reviews.apache.org/r/32329/#comment125172
should we have a test that asserts we get `AUTH_FAILED` if a user other
than deploysvc tries to kill the ads job?
src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
https://reviews.apache.org/r/32329/#comment125515
Presumably this is the result of an automated rename of
ShiroThriftInterceptor, but I think we really want this to be
ShiroAuthorizingParamInterceptor now, right?
src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java
https://reviews.apache.org/r/32329/#comment125520
s/public//
src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java
https://reviews.apache.org/r/32329/#comment125521
s/public// on all of these?
(and add @VisibleForTesting presumably if not just make them private
obviously).
src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java
https://reviews.apache.org/r/32329/#comment125531
nit: can we rename the `key` param to be `method` to avoid confusion with
the job keys that we deal in elsewhere in this class?
src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java
https://reviews.apache.org/r/32329/#comment125528
Would probably be helpful to add more detail to this exception (e.g. No
parameter annotated with ... found on method + method.getName())
src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java
https://reviews.apache.org/r/32329/#comment125534
Is this the right value here? We want to log, e.g., No path from
TaskConfigJobKey to JobKey?
src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java
https://reviews.apache.org/r/32329/#comment125517
As per the javadoc on this class, should we add validation that
authentication has happened and that the method returns a `Response`?
src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java
https://reviews.apache.org/r/32329/#comment125518
s/invocation.getMethod()/method
src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java
https://reviews.apache.org/r/32329/#comment125519
s/invocation.getMethod()/method
src/main/java/org/apache/aurora/scheduler/http/api/security/StructGetter.java
https://reviews.apache.org/r/32329/#comment125537
do we need javadoc here? (If so, should we update checkstyle to enforce
this?)
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
https://reviews.apache.org/r/32329/#comment125539
undo?
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
https://reviews.apache.org/r/32329/#comment125540
undo?
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
https://reviews.apache.org/r/32329/#comment125541
undo?
- Joshua Cohen
On March 23, 2015, 7:14 p.m., Kevin Sweeney wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32329/
---
(Updated March 23, 2015, 7:14 p.m.)
Review request for Aurora, Joshua Cohen and Bill Farner.
Bugs: AURORA-1187
https://issues.apache.org/jira/browse/AURORA-1187
Repository: aurora
Description
---
Apologies for the large diff, this wound up needing to input validation at
the AOP layer.
Probably the best place to start reading this diff is
Re: Review Request 32329: Extract job key from RPC parameters
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32329/#review77448 --- ~All minor stuff. config/pmd/custom.xml https://reviews.apache.org/r/32329/#comment125547 Please add a comment explaining the use case we're advocating with this. src/main/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityModule.java https://reviews.apache.org/r/32329/#comment125504 Seems like this value has meaning. It might be worth extracting a constant and document how it ties in with other components/configuration. src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthenticatingThriftInterceptor.java https://reviews.apache.org/r/32329/#comment125506 We don't do this in other interceptors. Seems like this is trading a NullPointerException for an IllegalStateException. I don't feel strongly, but i also wouldn't be upset if this code disappeared. src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthenticatingThriftInterceptor.java https://reviews.apache.org/r/32329/#comment125505 remove newline src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java https://reviews.apache.org/r/32329/#comment125507 This somewhat mirrors how authorization is done in some RPCs today, but i could imagine this being a surprise doen the road - that you have access to all affected jobs, but are denied. We briefly discussed this offline, but if you changed StructGetter to return `SetV` instead of `OptionalV`, you can restore expected behavior. src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java https://reviews.apache.org/r/32329/#comment125545 A doc would be helpful here. At first glance, it's odd that one method can produce multiple 'candidate methods'. src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java https://reviews.apache.org/r/32329/#comment125543 Added protection - filter and throw if there's != 1 annotated parameter. src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java https://reviews.apache.org/r/32329/#comment125542 Skip the Optional dance and push the throw up to annotatedParameterIndex. src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java https://reviews.apache.org/r/32329/#comment125546 Maybe a better message is No FieldGetter was supplied for x src/main/java/org/apache/aurora/scheduler/http/api/security/StructFieldGetter.java https://reviews.apache.org/r/32329/#comment125512 Consider s/Struct/Thrift/ src/main/java/org/apache/aurora/scheduler/http/api/security/StructGetter.java https://reviews.apache.org/r/32329/#comment125509 `Struct` doesn't seem like the right noun here. Perhaps `Field`? src/main/java/org/apache/aurora/scheduler/http/api/security/StructGetter.java https://reviews.apache.org/r/32329/#comment125508 I don't see any areas where this is used as a `Function`. If we're not getting anything from extending Function, i suggest you declare the method here and not extend. src/main/java/org/apache/aurora/scheduler/http/api/security/StructGetter.java https://reviews.apache.org/r/32329/#comment125510 Is the coupling to TBase necessary here? src/main/java/org/apache/aurora/scheduler/http/api/security/StructGetter.java https://reviews.apache.org/r/32329/#comment125511 Ditto. - Bill Farner On March 23, 2015, 7:14 p.m., Kevin Sweeney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32329/ --- (Updated March 23, 2015, 7:14 p.m.) Review request for Aurora, Joshua Cohen and Bill Farner. Bugs: AURORA-1187 https://issues.apache.org/jira/browse/AURORA-1187 Repository: aurora Description --- Apologies for the large diff, this wound up needing to input validation at the AOP layer. Probably the best place to start reading this diff is ApiSecurityIT to see the feature this patch enables. Diffs - config/pmd/custom.xml 521fd500146eb2e45f8e77c5c3c0cce330fedabb src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 827e85b6cac8bd52359610bbc2002973a769705c src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 2408cd1f9af5f109a339f5c78134465cb117f7fc src/main/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityModule.java cc9cfd38239f909b8a77bd1a773e31ec30130d41 src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java 808987939b2c4a850e488dc033b50b0178e95ba0
Re: Review Request 32329: Extract job key from RPC parameters
On March 23, 2015, 8:59 p.m., Joshua Cohen wrote:
Thanks, this is already much easier to follow.
One general question on the overall approach: do you think the DRY benefits
of using composed `StructFieldGetter`s to generate the functions that allow
walking from the starting type to `JobKey` outweigh the readability
improvements of using a fully explicit mapping? I.e.
FunctionJobUpdateRequest, JobKey UPDATE_REQUEST_TO_JOB_KEY = new
Function... {
@Override
public JobKey apply(JobUpdateRequest request) {
return request.getTaskConfig().getJobKey();
}
};
I raised this question offline as well. It's not clear to me that this
DRY-ness is worth the complexity. Required null checking in the call chain is
one downside to the more direct approach you illustrated.
- Bill
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32329/#review77289
---
On March 23, 2015, 7:14 p.m., Kevin Sweeney wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32329/
---
(Updated March 23, 2015, 7:14 p.m.)
Review request for Aurora, Joshua Cohen and Bill Farner.
Bugs: AURORA-1187
https://issues.apache.org/jira/browse/AURORA-1187
Repository: aurora
Description
---
Apologies for the large diff, this wound up needing to input validation at
the AOP layer.
Probably the best place to start reading this diff is ApiSecurityIT to see
the feature this patch enables.
Diffs
-
config/pmd/custom.xml 521fd500146eb2e45f8e77c5c3c0cce330fedabb
src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java
827e85b6cac8bd52359610bbc2002973a769705c
src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java
2408cd1f9af5f109a339f5c78134465cb117f7fc
src/main/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityModule.java
cc9cfd38239f909b8a77bd1a773e31ec30130d41
src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
808987939b2c4a850e488dc033b50b0178e95ba0
src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthenticatingThriftInterceptor.java
PRE-CREATION
src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java
PRE-CREATION
src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroThriftInterceptor.java
4e341e05c34b1be38f0040c26b671a0cc797a771
src/main/java/org/apache/aurora/scheduler/http/api/security/StructFieldGetter.java
PRE-CREATION
src/main/java/org/apache/aurora/scheduler/http/api/security/StructGetter.java
PRE-CREATION
src/main/java/org/apache/aurora/scheduler/http/api/security/StructGetters.java
PRE-CREATION
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
5588d1793d6713ee4581ac9f938d9a8689acb315
src/main/java/org/apache/aurora/scheduler/thrift/aop/AopModule.java
bdd2185f3a7a94b39bcec3c73455e970d87f0c6a
src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java
cafd10f6b705568588c1b92644b482003242fe2e
src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java
ed284f46ac8f01bd6d9e317f995f16d6e666a68d
src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java
76cb691e6d7d4fada3a18fde73aceed7039bcaa4
src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthenticatingThriftInterceptorTest.java
PRE-CREATION
src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java
PRE-CREATION
src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroThriftInterceptorTest.java
d2ba2730c4509dc9a636fd32e9244b0d7fa2884f
src/test/java/org/apache/aurora/scheduler/http/api/security/StructFieldGetterTest.java
PRE-CREATION
src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java
1f24e7d47e1f777ffef19a73d01171fcacd31cdb
src/test/java/org/apache/aurora/scheduler/thrift/aop/AopModuleTest.java
d20c9da3c4944ec8c50fe8d48b7e459ff1c7082b
Diff: https://reviews.apache.org/r/32329/diff/
Testing
---
./gradlew -Pq build
Thanks,
Kevin Sweeney
Re: Review Request 32329: Extract job key from RPC parameters
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32329/ --- (Updated March 20, 2015, 5:16 p.m.) Review request for Aurora, Joshua Cohen and Bill Farner. Bugs: AURORA-1187 https://issues.apache.org/jira/browse/AURORA-1187 Repository: aurora Description --- Apologies for the large diff, this wound up needing to input validation at the AOP layer. Probably the best place to start reading this diff is ApiSecurityIT to see the feature this patch enables. Also the best place to start reviewing FieldGetterDag is also its tests and usage. Diffs (updated) - config/pmd/custom.xml 521fd500146eb2e45f8e77c5c3c0cce330fedabb src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 827e85b6cac8bd52359610bbc2002973a769705c src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 2408cd1f9af5f109a339f5c78134465cb117f7fc src/main/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityModule.java cc9cfd38239f909b8a77bd1a773e31ec30130d41 src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java 808987939b2c4a850e488dc033b50b0178e95ba0 src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthenticatingThriftInterceptor.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroThriftInterceptor.java 4e341e05c34b1be38f0040c26b671a0cc797a771 src/main/java/org/apache/aurora/scheduler/http/api/security/StructFieldGetter.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/StructGetter.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/StructGetters.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 5588d1793d6713ee4581ac9f938d9a8689acb315 src/main/java/org/apache/aurora/scheduler/thrift/aop/AopModule.java bdd2185f3a7a94b39bcec3c73455e970d87f0c6a src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java cafd10f6b705568588c1b92644b482003242fe2e src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java ed284f46ac8f01bd6d9e317f995f16d6e666a68d src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java 76cb691e6d7d4fada3a18fde73aceed7039bcaa4 src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthenticatingThriftInterceptorTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroThriftInterceptorTest.java d2ba2730c4509dc9a636fd32e9244b0d7fa2884f src/test/java/org/apache/aurora/scheduler/http/api/security/StructFieldGetterTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java 1f24e7d47e1f777ffef19a73d01171fcacd31cdb src/test/java/org/apache/aurora/scheduler/thrift/aop/AopModuleTest.java d20c9da3c4944ec8c50fe8d48b7e459ff1c7082b Diff: https://reviews.apache.org/r/32329/diff/ Testing --- ./gradlew -Pq build Thanks, Kevin Sweeney
Re: Review Request 32329: Extract job key from RPC parameters
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32329/#review77319 --- Ship it! Master (a3a35e9) is green with this patch. ./build-support/jenkins/build.sh I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On March 21, 2015, 12:16 a.m., Kevin Sweeney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32329/ --- (Updated March 21, 2015, 12:16 a.m.) Review request for Aurora, Joshua Cohen and Bill Farner. Bugs: AURORA-1187 https://issues.apache.org/jira/browse/AURORA-1187 Repository: aurora Description --- Apologies for the large diff, this wound up needing to input validation at the AOP layer. Probably the best place to start reading this diff is ApiSecurityIT to see the feature this patch enables. Also the best place to start reviewing FieldGetterDag is also its tests and usage. Diffs - config/pmd/custom.xml 521fd500146eb2e45f8e77c5c3c0cce330fedabb src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 827e85b6cac8bd52359610bbc2002973a769705c src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 2408cd1f9af5f109a339f5c78134465cb117f7fc src/main/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityModule.java cc9cfd38239f909b8a77bd1a773e31ec30130d41 src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java 808987939b2c4a850e488dc033b50b0178e95ba0 src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthenticatingThriftInterceptor.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroThriftInterceptor.java 4e341e05c34b1be38f0040c26b671a0cc797a771 src/main/java/org/apache/aurora/scheduler/http/api/security/StructFieldGetter.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/StructGetter.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/StructGetters.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 5588d1793d6713ee4581ac9f938d9a8689acb315 src/main/java/org/apache/aurora/scheduler/thrift/aop/AopModule.java bdd2185f3a7a94b39bcec3c73455e970d87f0c6a src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java cafd10f6b705568588c1b92644b482003242fe2e src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java ed284f46ac8f01bd6d9e317f995f16d6e666a68d src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java 76cb691e6d7d4fada3a18fde73aceed7039bcaa4 src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthenticatingThriftInterceptorTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroThriftInterceptorTest.java d2ba2730c4509dc9a636fd32e9244b0d7fa2884f src/test/java/org/apache/aurora/scheduler/http/api/security/StructFieldGetterTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java 1f24e7d47e1f777ffef19a73d01171fcacd31cdb src/test/java/org/apache/aurora/scheduler/thrift/aop/AopModuleTest.java d20c9da3c4944ec8c50fe8d48b7e459ff1c7082b Diff: https://reviews.apache.org/r/32329/diff/ Testing --- ./gradlew -Pq build Thanks, Kevin Sweeney
Re: Review Request 32329: Extract job key from RPC parameters
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32329/#review77273 --- Master (f12d9fe) is red with this patch. ./build-support/jenkins/build.sh :startScripts :distTar :distZip :assemble :compileJmhJava :processJmhResources UP-TO-DATE :jmhClasses :checkstyleJmh :jsHint :checkstyleMain :compileTestJava/home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/java/org/apache/aurora/scheduler/http/api/security/FieldGetterDagTest.java:22: error: cannot find symbol import org.apache.aurora.gen.storage.StoredJob; ^ symbol: class StoredJob location: package org.apache.aurora.gen.storage /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/java/org/apache/aurora/scheduler/http/api/security/FieldGetterDagTest.java:22: error: cannot find symbol import org.apache.aurora.gen.storage.StoredJob; ^ symbol: class StoredJob location: package org.apache.aurora.gen.storage /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/java/org/apache/aurora/scheduler/http/api/security/FieldGetterDagTest.java:63: error: cannot find symbol assertNull(jobKeyFieldGetterDag.getterFor(StoredJob.class).orNull()); ^ symbol: class StoredJob location: class FieldGetterDagTest 2 errors FAILED FAILURE: Build failed with an exception. * What went wrong: Execution failed for task ':compileTestJava'. Compilation failed; see the compiler error output for details. * Try: Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. BUILD FAILED Total time: 1 mins 47.53 secs I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On March 20, 2015, 8:23 p.m., Kevin Sweeney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32329/ --- (Updated March 20, 2015, 8:23 p.m.) Review request for Aurora, Joshua Cohen and Bill Farner. Bugs: AURORA-1187 https://issues.apache.org/jira/browse/AURORA-1187 Repository: aurora Description --- Apologies for the large diff, this wound up needing to input validation at the AOP layer. Probably the best place to start reading this diff is ApiSecurityIT to see the feature this patch enables. Also the best place to start reviewing FieldGetterDag is also its tests and usage. Diffs - config/pmd/custom.xml 521fd500146eb2e45f8e77c5c3c0cce330fedabb src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 827e85b6cac8bd52359610bbc2002973a769705c src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 2408cd1f9af5f109a339f5c78134465cb117f7fc src/main/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityModule.java cc9cfd38239f909b8a77bd1a773e31ec30130d41 src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java 808987939b2c4a850e488dc033b50b0178e95ba0 src/main/java/org/apache/aurora/scheduler/http/api/security/FieldGetterDag.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthenticatingThriftInterceptor.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroThriftInterceptor.java 4e341e05c34b1be38f0040c26b671a0cc797a771 src/main/java/org/apache/aurora/scheduler/http/api/security/StructFieldGetter.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/StructGetter.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/StructGetters.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 5588d1793d6713ee4581ac9f938d9a8689acb315 src/main/java/org/apache/aurora/scheduler/thrift/aop/AopModule.java bdd2185f3a7a94b39bcec3c73455e970d87f0c6a src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java cafd10f6b705568588c1b92644b482003242fe2e src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java ed284f46ac8f01bd6d9e317f995f16d6e666a68d src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java 76cb691e6d7d4fada3a18fde73aceed7039bcaa4 src/test/java/org/apache/aurora/scheduler/http/api/security/FieldGetterDagTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthenticatingThriftInterceptorTest.java PRE-CREATION
Review Request 32329: Extract job key from RPC parameters
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32329/ --- Review request for Aurora, Joshua Cohen and Bill Farner. Bugs: AURORA-1187 https://issues.apache.org/jira/browse/AURORA-1187 Repository: aurora Description --- Apologies for the large diff, this wound up needing to input validation at the AOP layer. Probably the best place to start reading this diff is ApiSecurityIT to see the feature this patch enables. Also the best place to start reviewing FieldGetterDag is also its tests and usage. Diffs - config/pmd/custom.xml 521fd500146eb2e45f8e77c5c3c0cce330fedabb src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 827e85b6cac8bd52359610bbc2002973a769705c src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 2408cd1f9af5f109a339f5c78134465cb117f7fc src/main/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityModule.java cc9cfd38239f909b8a77bd1a773e31ec30130d41 src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java 808987939b2c4a850e488dc033b50b0178e95ba0 src/main/java/org/apache/aurora/scheduler/http/api/security/FieldGetterDag.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthenticatingThriftInterceptor.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroThriftInterceptor.java 4e341e05c34b1be38f0040c26b671a0cc797a771 src/main/java/org/apache/aurora/scheduler/http/api/security/StructFieldGetter.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/StructGetter.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/StructGetters.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 5588d1793d6713ee4581ac9f938d9a8689acb315 src/main/java/org/apache/aurora/scheduler/thrift/aop/AopModule.java bdd2185f3a7a94b39bcec3c73455e970d87f0c6a src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java cafd10f6b705568588c1b92644b482003242fe2e src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java ed284f46ac8f01bd6d9e317f995f16d6e666a68d src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java 76cb691e6d7d4fada3a18fde73aceed7039bcaa4 src/test/java/org/apache/aurora/scheduler/http/api/security/FieldGetterDagTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthenticatingThriftInterceptorTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroThriftInterceptorTest.java d2ba2730c4509dc9a636fd32e9244b0d7fa2884f src/test/java/org/apache/aurora/scheduler/http/api/security/StructFieldGetterTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java 1f24e7d47e1f777ffef19a73d01171fcacd31cdb src/test/java/org/apache/aurora/scheduler/thrift/aop/AopModuleTest.java d20c9da3c4944ec8c50fe8d48b7e459ff1c7082b Diff: https://reviews.apache.org/r/32329/diff/ Testing --- ./gradlew -Pq build Thanks, Kevin Sweeney
Re: Review Request 32329: Extract job key from RPC parameters
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32329/ --- (Updated March 20, 2015, 3:01 p.m.) Review request for Aurora, Joshua Cohen and Bill Farner. Changes --- stop referencing nonexistent StoredJob. Bugs: AURORA-1187 https://issues.apache.org/jira/browse/AURORA-1187 Repository: aurora Description --- Apologies for the large diff, this wound up needing to input validation at the AOP layer. Probably the best place to start reading this diff is ApiSecurityIT to see the feature this patch enables. Also the best place to start reviewing FieldGetterDag is also its tests and usage. Diffs (updated) - config/pmd/custom.xml 521fd500146eb2e45f8e77c5c3c0cce330fedabb src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 827e85b6cac8bd52359610bbc2002973a769705c src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 2408cd1f9af5f109a339f5c78134465cb117f7fc src/main/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityModule.java cc9cfd38239f909b8a77bd1a773e31ec30130d41 src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java 808987939b2c4a850e488dc033b50b0178e95ba0 src/main/java/org/apache/aurora/scheduler/http/api/security/FieldGetterDag.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthenticatingThriftInterceptor.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroThriftInterceptor.java 4e341e05c34b1be38f0040c26b671a0cc797a771 src/main/java/org/apache/aurora/scheduler/http/api/security/StructFieldGetter.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/StructGetter.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/StructGetters.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 5588d1793d6713ee4581ac9f938d9a8689acb315 src/main/java/org/apache/aurora/scheduler/thrift/aop/AopModule.java bdd2185f3a7a94b39bcec3c73455e970d87f0c6a src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java cafd10f6b705568588c1b92644b482003242fe2e src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java ed284f46ac8f01bd6d9e317f995f16d6e666a68d src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java 76cb691e6d7d4fada3a18fde73aceed7039bcaa4 src/test/java/org/apache/aurora/scheduler/http/api/security/FieldGetterDagTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthenticatingThriftInterceptorTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroThriftInterceptorTest.java d2ba2730c4509dc9a636fd32e9244b0d7fa2884f src/test/java/org/apache/aurora/scheduler/http/api/security/StructFieldGetterTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java 1f24e7d47e1f777ffef19a73d01171fcacd31cdb src/test/java/org/apache/aurora/scheduler/thrift/aop/AopModuleTest.java d20c9da3c4944ec8c50fe8d48b7e459ff1c7082b Diff: https://reviews.apache.org/r/32329/diff/ Testing --- ./gradlew -Pq build Thanks, Kevin Sweeney
Re: Review Request 32329: Extract job key from RPC parameters
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32329/#review77286 --- Ship it! Master (f12d9fe) is green with this patch. ./build-support/jenkins/build.sh I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On March 20, 2015, 10:01 p.m., Kevin Sweeney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32329/ --- (Updated March 20, 2015, 10:01 p.m.) Review request for Aurora, Joshua Cohen and Bill Farner. Bugs: AURORA-1187 https://issues.apache.org/jira/browse/AURORA-1187 Repository: aurora Description --- Apologies for the large diff, this wound up needing to input validation at the AOP layer. Probably the best place to start reading this diff is ApiSecurityIT to see the feature this patch enables. Also the best place to start reviewing FieldGetterDag is also its tests and usage. Diffs - config/pmd/custom.xml 521fd500146eb2e45f8e77c5c3c0cce330fedabb src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 827e85b6cac8bd52359610bbc2002973a769705c src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 2408cd1f9af5f109a339f5c78134465cb117f7fc src/main/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityModule.java cc9cfd38239f909b8a77bd1a773e31ec30130d41 src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java 808987939b2c4a850e488dc033b50b0178e95ba0 src/main/java/org/apache/aurora/scheduler/http/api/security/FieldGetterDag.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthenticatingThriftInterceptor.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroThriftInterceptor.java 4e341e05c34b1be38f0040c26b671a0cc797a771 src/main/java/org/apache/aurora/scheduler/http/api/security/StructFieldGetter.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/StructGetter.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/api/security/StructGetters.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 5588d1793d6713ee4581ac9f938d9a8689acb315 src/main/java/org/apache/aurora/scheduler/thrift/aop/AopModule.java bdd2185f3a7a94b39bcec3c73455e970d87f0c6a src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java cafd10f6b705568588c1b92644b482003242fe2e src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java ed284f46ac8f01bd6d9e317f995f16d6e666a68d src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java 76cb691e6d7d4fada3a18fde73aceed7039bcaa4 src/test/java/org/apache/aurora/scheduler/http/api/security/FieldGetterDagTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthenticatingThriftInterceptorTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroThriftInterceptorTest.java d2ba2730c4509dc9a636fd32e9244b0d7fa2884f src/test/java/org/apache/aurora/scheduler/http/api/security/StructFieldGetterTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java 1f24e7d47e1f777ffef19a73d01171fcacd31cdb src/test/java/org/apache/aurora/scheduler/thrift/aop/AopModuleTest.java d20c9da3c4944ec8c50fe8d48b7e459ff1c7082b Diff: https://reviews.apache.org/r/32329/diff/ Testing --- ./gradlew -Pq build Thanks, Kevin Sweeney
