Management::get method does not reflect maskable configs

[li-boxuan]

This is modified from the existing test.

    @Test
    public void updateConfigurationTest() throws Exception {
        final MapConfiguration graphConfig = getGraphConfig();
        final String graphName = graphConfig.getString(GRAPH_NAME.toStringWithoutRoot());

        try {
            ConfiguredGraphFactory.createConfiguration(graphConfig);
            final StandardJanusGraph graph = (StandardJanusGraph) ConfiguredGraphFactory.open(graphName);
            assertNotNull(graph);

            final Map<String, Object> map = graphConfig.getMap();
            map.put("query.batch", true);
            ConfiguredGraphFactory.updateConfiguration(graphName, ConfigurationUtil.loadMapConfiguration(map));
            assertNull(gm.getGraph(graphName));

            JanusGraph graph2 = ConfiguredGraphFactory.open(graphName);
            // THIS FAILS! It still returns the default value, false
            assertEquals("true", graph2.openManagement().get("query.batch"));
        } finally {
            ConfiguredGraphFactory.removeConfiguration(graphName);
            ConfiguredGraphFactory.close(graphName);
        }
    }

Compared to the existing test (that is passing!) where we modify https://github.com/JanusGraph/janusgraph/blob/ba93c4bfda9e4c72b01aae7dc2ee9c3c5b5f37d1/janusgraph-backend-testutils/src/main/java/org/janusgraph/core/AbstractConfiguredGraphFactoryTest.java#L228 in the above example, we modify query.batch option. Somehow the change is not reflected.

[li-boxuan]

Aha, it seems the updated configuration DOES apply. mgmt.get() does not reflect the current configurations which can be very confusing and misleading.

[li-boxuan]

Actually, even if we use a configuration file and JanusGraphFactory.open(file), we would observe the same phenomenon for maskable properties. So this is not only related to ConfiguredGraphFactory.

[cdegroc]

I could be missing your point, but isn't it because:

1. ConfiguredGraphFactory.updateConfiguration(graphName, ConfigurationUtil.loadMapConfiguration(map)); updates the graph's configuration (within the ConfiguredGraphFactory)
2. assertEquals("true", graph2.openManagement().get("query.batch")); checks the global configuration (not the graph's configuration from the ConfiguredGraphFactory)

Concretely, if we updated the assertion as follows, the test would pass:

@Test
public void updateConfigurationTest() throws Exception {
    final MapConfiguration graphConfig = getGraphConfig();
    final String graphName = graphConfig.getString(GRAPH_NAME.toStringWithoutRoot());

    try {
        ConfiguredGraphFactory.createConfiguration(graphConfig);
        final StandardJanusGraph graph = (StandardJanusGraph) ConfiguredGraphFactory.open(graphName);
        assertNotNull(graph);

        final Map<String, Object> map = graphConfig.getMap();
        map.put("query.batch", true);
        ConfiguredGraphFactory.updateConfiguration(graphName, ConfigurationUtil.loadMapConfiguration(map));
        assertNull(gm.getGraph(graphName));

        // PASSES
        assertEquals(true, ConfiguredGraphFactory.getConfiguration(graphName).get("query.batch"));
        
        // PASSES
        final StandardJanusGraph graph2 = (StandardJanusGraph) ConfiguredGraphFactory.open(graphName);
        Configuration graphConfiguration = graph2.getConfiguration().getConfiguration();
        assertEquals(true, graphConfiguration.get(org.janusgraph.graphdb.configuration.GraphDatabaseConfiguration.USE_MULTIQUERY));
    } finally {
        ConfiguredGraphFactory.removeConfiguration(graphName);
        ConfiguredGraphFactory.close(graphName);
    }
}

[li-boxuan]

assertEquals("true", graph2.openManagement().get("query.batch")); checks the global configuration

Yeah, you are right. That's somewhat confusing, isn't it? We might want to document it otherwise people like me would take it for granted that one can achieve current configs using mgmt.get.

[cdegroc]

assertEquals("true", graph2.openManagement().get("query.batch")); checks the global configuration

Yeah, you are right. That's somewhat confusing, isn't it? We might want to document it otherwise people like me would take it for granted that one can achieve current configs using mgmt.get.

I agree this is very confusing (also to me) and should be fixed, or documented if it can't be fixed.