Core: Adjust namespace separator in TestRESTCatalog

[gaborkaszab]

The resource paths used for comparisons in the REST catalog test use different namespace separator than the test REST catalogs. This PR brings them in line.

[gaborkaszab]

When we get the path in the REST catalog tests we use e.g. TestRESTCatalog.RESOURCE_PATHS that uses %1F as namespace separator. However, the test REST catalog use %2E as NS separator, so verification on paths being called fail if there is a multi-level namespace, or metadata table name involved.
One workaround can be to create a new ResourcePaths instance with the particular NS-separator for each such test (one example is in testNotModified()) but I think it's more future-proof to configure the RESOURCE_PATHS member to use such separator. For instance I ran into this issue when rebasing my freshness-aware loading PR and then some of my path checks started failing.

[gaborkaszab]

cc @nastra as the owner of the configurable namespace separator change

[nastra]

it seems like what you're really trying to test is

  @Test
  public void nestedNamespaceWithLegacyAndNewSeparator() {
    Namespace namespace = Namespace.of("first", "second", "third");
    String legacySeparator = RESTUtil.NAMESPACE_SEPARATOR_URLENCODED_UTF_8;
    String newSeparator = "%2E";

    // legacy separator is always used by default, so no need to configure it
    ResourcePaths withLegacySeparator = ResourcePaths.forCatalogProperties(ImmutableMap.of());
    ResourcePaths withNewSeparator =
        ResourcePaths.forCatalogProperties(
            ImmutableMap.of(RESTCatalogProperties.NAMESPACE_SEPARATOR, newSeparator));

    // old client would call encodeNamespace without specifying a separator when constructing a
    // resource path
    String legacyEncodedNamespace = RESTUtil.encodeNamespace(namespace);
    assertThat(withLegacySeparator.namespace(namespace))
        .contains(legacyEncodedNamespace)
        .contains(legacySeparator);

    // old client would decode without specifying the separator
    assertThat(RESTUtil.decodeNamespace(legacyEncodedNamespace)).isEqualTo(namespace);

    // new server would decode the namespace using the new separator
    assertThat(RESTUtil.decodeNamespace(legacyEncodedNamespace, newSeparator)).isEqualTo(namespace);

    // new client would call encodeNamespace with specifying a separator when constructing a
    // resource path
    String newEncodedSeparator = RESTUtil.encodeNamespace(namespace, newSeparator);
    assertThat(withNewSeparator.namespace(namespace))
        .contains(newEncodedSeparator)
        .contains(newSeparator);

    // new server would decode the namespace using the new separator
    assertThat(RESTUtil.decodeNamespace(newEncodedSeparator, newSeparator)).isEqualTo(namespace);
  }

In that case the test should probably live in TestResourcePaths

[nastra]

@gaborkaszab can you also add ^ to TestResourcePaths

[gaborkaszab]

Sure. I've split this into 2

[nastra]

Suggested change

	// Do verification with paths using the default namespace separator.
	// Do verification with paths using the legacy namespace separator.

[gaborkaszab]

In fact with the suggested naming, I think this comment is redundant, so I'm removing it.

[nastra]

you might want to add a comment that says that the server now always sends back the separator in the config response so that this is clear here

[gaborkaszab]

Done. I added an extra check also to verify the override is sent by the adapter. Makes the test more future proof, however, this may be an overkill. Let me know and I'll remove the check

[nastra]

we don't really need the table creation here

[gaborkaszab]

I use this so that I can also verify that the paths use the desired namespace separators. I know you've commented that it rather belongs to TestResourcePaths, but in general I think whenever an end-to-end scenario is easy to do, it makes sense to add test coverage there (besides narrower unit testing). In this case it's pretty simple to cover, and add more test coverage not just to ResourcePaths but also to the RESTCatalog code around creating and using the paths. This gives use more complete tests here: the paths and the params together.

[nastra]

you can just use containsEntry(...) directly here

[gaborkaszab]

Done

[gaborkaszab]

Thanks for looking, @nastra !

[gaborkaszab]

Done

[gaborkaszab]

In fact with the suggested naming, I think this comment is redundant, so I'm removing it.

[gaborkaszab]

Sure. I've split this into 2

[gaborkaszab]

Thanks for reviewing, @nastra !