Skip to content

Commit e3f320e

Browse files
committed
GROOVY-12267: Remove the dead declared-method check from JMX operation export
buildOperationMapFrom guarded operation export with (declaredMethods.contains(name) && !OPS_EXCEPTION_LIST.contains(name)) || (!OPS_EXCEPTION_LIST.contains(name)) which reduces to its right operand, so the declaredMethods lookup above it was computed and never used and the comment claiming it avoided methods from parents described something the code did not do. Drop the dead operand and the unused lookup, and state the actual contract in the javadoc: the default export spans the whole inheritance chain, and a caller wanting a narrower set names the operations explicitly in a descriptor. Behaviour is unchanged. Making the declared-method check live instead was considered and rejected: JmxBuilder's own embedded-descriptor fixtures declare their operations on a base class, and 'operations: "*"' routes to this method and has to mean all of them, so restricting to declared methods breaks four existing tests and gives '*' a meaning it should not have. Adds two tests pinning behaviour that was previously untested: inherited operations are exported, and getMetaClass/setMetaClass are not, the latter because they resolve to the metaClass MetaProperty and are dropped by the getter/setter filter.
1 parent c32090e commit e3f320e

2 files changed

Lines changed: 31 additions & 4 deletions

File tree

subprojects/groovy-jmx/src/main/groovy/groovy/jmx/builder/JmxMetaMapBuilder.groovy

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -389,18 +389,20 @@ class JmxMetaMapBuilder {
389389
* **************************************/
390390
/**
391391
* Returns a meta map of operations from given object.
392+
* <p>
393+
* Every public method reachable on the object is exported, inherited ones included, less
394+
* the names in {@code OPS_EXCEPTION_LIST}. To export a narrower set, name the wanted
395+
* operations explicitly in a descriptor rather than relying on this default.
396+
*
392397
* @param object to profile
393398
* @return The meta map generated.
394399
*/
395400
static Map buildOperationMapFrom(def object) {
396401
def methods = object.metaClass.methods
397402
def ops = [:]
398403

399-
def declaredMethods = object.class.declaredMethods*.name
400-
401404
methods.each { method ->
402-
// avoid picking up extra methods from parents
403-
if ((declaredMethods.contains(method.name) && !OPS_EXCEPTION_LIST.contains(method.name)) || (!OPS_EXCEPTION_LIST.contains(method.name))) {
405+
if (!OPS_EXCEPTION_LIST.contains(method.name)) {
404406
String mName = method.name
405407
MetaProperty prop =
406408
(mName.length() > 3 && (mName.startsWith("get") || mName.startsWith("set")) ||

subprojects/groovy-jmx/src/test/groovy/groovy/jmx/builder/JmxMetaMapBuilderTest.groovy

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -340,6 +340,31 @@ class JmxMetaMapBuilderTest {
340340
assert map."set".name == "set"
341341
}
342342

343+
@Test
344+
void testMetaClassAccessorsAreNotExportedAsOperations() {
345+
// Not exported because getMetaClass/setMetaClass resolve to the metaClass MetaProperty
346+
// and are dropped by the getter/setter filter, not because they are named in
347+
// OPS_EXCEPTION_LIST. Pinned so that a change to that filter cannot quietly publish
348+
// them as remotely invokable operations.
349+
[new MockManagedObject(), new MockManagedGroovyObject()].each { object ->
350+
def map = JmxMetaMapBuilder.buildOperationMapFrom(object)
351+
assert !map."getMetaClass"
352+
assert !map."setMetaClass"
353+
}
354+
}
355+
356+
@Test
357+
void testInheritedOperationsRemainExported() {
358+
// The default export deliberately spans the whole inheritance chain: JmxBuilder's own
359+
// embedded-descriptor fixtures declare their operations on a base class, and
360+
// 'operations: "*"' routes here and must mean all of them.
361+
def map = JmxMetaMapBuilder.buildOperationMapFrom(new EmbeddedAllOps())
362+
363+
assert map."doNothing"
364+
assert map."doTwoThings"
365+
assert map."doThreeThings"
366+
}
367+
343368
@Test
344369
void testBuildOperationFromDescriptorMap() {
345370
def object = new MockManagedObject()

0 commit comments

Comments
 (0)