SKA-SP-5555 by Zarquan · Pull Request #213 · opencadc/reg

Zarquan · 2025-08-04T06:28:42Z

Added support to the client for multiple registry endpoints.
See:

Repeating the baseURL configuration property will add additional registry endpoints.

# configure RegistryClient
ca.nrc.cadc.reg.client.RegistryClient.baseURL = https://registry-one/
ca.nrc.cadc.reg.client.RegistryClient.baseURL = https://registry-two/
ca.nrc.cadc.reg.client.RegistryClient.baseURL = https://registry-three/

The client will query the registries in sequence and use the first valid http response it gets.

Added a new set of JUnit tests that use MockServer to create test services that mimic local registry services allowing us to test specific scenarios including dropped connections, http timeouts and resource not found errors.

pdowler

Initial code review but not complete yet.

I haven't looked at the mock tests but my experience in the past with mocks (easymock) has not been good. In that case, we end up with a lot of brittle test code that mimics the implementation and breaks when you improve anything. I haven't looked closely at mock-server and the test code you added so I don't have anything specific to say on it yet... but that's a lot of test code (yeah: testing failure modes is hard). I'll try to look at the test code in the next day or so.

pdowler · 2025-08-05T17:21:17Z

+     * @param configFile The configuration file to use.
+     * 
+     */
+    public RegistryClient(final File configFile) {


A special constructor for test code is fine, but it should be package access; just a plain old // comment that this is for test use is fine.

I prefer this one that takes File since that maps directly to the PropertiesReader ctor; might as well flatten the call to this(...) into a single line.

Above, the no arg ctor could also be just this(new PropertiesReader(CONFIG_FILE_NAME)) and then the extra ctor below is not needed.

Last, the ctor RegistryClient(final PropertiesReader propReader) should be private.

this constructor should still not be public (package access for use by tests only)

pdowler · 2025-08-05T17:26:48Z


-    private URL regBaseURL;
-    private String capsDomain;
+    private List<URL> regBaseURLs = new ArrayList<URL>();


The regbaseURLs field should be declared final so it can never be null.

pdowler · 2025-08-05T17:31:55Z

+                continue;
+            }
+            List<String> values = mvp.getProperty(uri.toString());
+            if (values == null) {


this check could be if (values == null || values.isEmpty()) since the result is the same; this is just defensive so it doesn't depend on what MultiValuedProperties returns for "not found".

pdowler · 2025-08-05T17:41:34Z

+                String str = values.get(0);
+                URL url = new URL(str);
+                return url;
+            } catch (MalformedURLException e) {


This used to fail (RuntimeException) if it encountered bad content... now it silently skips bad content and probably fails with a ResourceNotFoundException at the bottom.

At a minimum, the logging should be warn or no one will notice the bad content; realistically, they still won't notice the bad content. I wonder is this strategy would work: create the exception by instead of throw just assign to a local variable. That could be done for all the failure modes exception "not found".

At the bottom of the method, either throw the (first) exception that was created an fall back to the ResourceNotFoundException there now. That way failures from duplicate entries (size() > 1) or bad URLs won't be entirely hidden and just appear to be "not found". This kind of error handling comes up a lot when code has alternatives to try... and generally reporting the first failure is the right way to handle it.

I've restructured the exception handling so it still throws ResourceNotFoundException but it attaches the first exception from the loop as the cause.
I've kept a list of the other Exceptions in case we want to do something more with them later. It could be simplified to just keeping one Exception, but we would have to have lots of if not set tests making the code more complex.

pdowler · 2025-08-05T17:45:35Z

+     * @throws IOException
+     * 
+     */
+    public void deleteCache()


Although this is nominally for test convenience, I have considered making it part of the API before to enable some other use cases so this can stay public.

I explicitly called it deleteCache() rather than clearCache() because it deletes the directory.

pdowler · 2025-08-05T17:49:35Z

    }

+    /*
+     * This no longer works because endpoint address varies. 


I don't see why this test should fail now. It should work as long as one points the config dir that will be used at a place that does not have the default config file (that's the intended use of the system prop: running local tests and pointing them at a specific reg service). I think this can still work but maybe it made setup/assumptions that aren't right??

pdowler · 2025-08-05T17:54:07Z

+     */
+
+    /*
+     * This no longer works because endpoint address varies. 


This test is making sure that a very old behaviour (way back: system prop could override config rather than just "provide" config) so it test the if/else in the constructor. It should also be able to work as long as there is a config file to verify the system prop was ignored.

Zarquan · 2025-08-15T05:59:07Z

Hopefully the mock tests aren't too brittle. They are very verbose but they should follow a consistent pattern. They are designed to be self contained with everything in the Java code for each test rather than relying on having lots of different resource and configuration files.
I learned a lot about the behaviour of the cache and http connector while developing the tests.

pdowler · 2025-08-19T22:24:52Z

+        Log4jInit.setLevel("ca.nrc.cadc.net", Level.DEBUG);
+    }
+
+    //private static ClientAndServer proxyServer;


Only commenting on this first test class, but this is probably generally applicable...

It feels like the mockServer and the static start and stop methods could be in a base test class so they didn't have to be repeated in all the individual test classes.

I would have probably tried to repurpose a single setupMockServer() with a more elaborate setup and then implement more @Test methods using that setup (probably just varying the registry client config file). I think it would be less code but a more complex setup... hard to assess the brittleness and maintenance in the future though. I think it might at least be good for reducing to 3 test classes (Single, Mixed, Multiple).

Whenever there is xml to return from a mock request, I would have an xml file (in src/test/resources) and use a utility to read it. We use something like this a lot:

File f = FileUtil.getFileFromResource(filename, XYZTest.class); String s = StringUtil.readFromInputStream(new FileInputStream(f), "UTF-8");

for such things. (Both of those are in cadc-util). Putting those files in src/test/resources makes then easier to edit and re-use. Specifically, I am thinking about the availability and capabilities xml needed for returned body in some setups... dynamically creating the resource-caps output in code is fine because those are one-liners and plain text.

Fixed (in new branch)

pdowler · 2025-08-19T22:27:45Z

+        PrintWriter printWriter = new PrintWriter(
+            new FileWriter(configFile)
+            );
+        printWriter.print(


cosmetic: multiple calls to println("...") would avoid string concat, manual line breaks, and one line of code that spans multiple lines (which I don't like to do unless I have to).

Fixed (in new branch)

pdowler · 2025-08-19T22:31:01Z

+        try {
+            Capabilities capabilities = registryClient.getCapabilities(
+                new URI("ivo://good.authority/good-service")
+                );


Good assertions here (and elsewhere):

assertNotNull(capabilities); assertFalse(capabilities.getCapabilities().isEmpty());

This would fail with assertion rather than NullPointerException and be less brittle if the xml doc changes for some reason.

Fixed (in new branch)

pdowler · 2025-08-19T22:37:32Z

+        // and again to try to get the content without using the cache.
+        // In fact MockServer will log multiple requests because the HttpGet library retries the request multiple times.
+        mockServer.verify(
+            request()


I would probably collapse this to a single line (I think checkstyle allows pretty long lines since we don't live on 80-char terminals anymore). Something like:

mockServer.verify( request().withPath("/bad-registry-one/resource-caps"), VerificationTimes.atLeast(2) );

is pretty easy on the eyes and still allows mockServer.verify to stand out.

Fixed (in new branch)

Zarquan · 2025-09-25T07:36:55Z

Closing this one before I open a new pull request for a new branch.
Comments and suggestions are fixed in the new branch.

Zarquan added 22 commits August 4, 2025 06:40

Support for multiple endpoints

b01d5e6

Making it easier to test

aa86302

Using Mock to test offline

9910a08

Adding test properties

6f2db2f

Update dependencies

702fa0a

Moved directory delete into the client

508c76c

Removed stack trace from debug message

1979f2a

Added MockServer tests

d4493b8

Added mixed registries tests

bd2592e

Typo fix

66affc9

Typo fix

54a411c

Refactoring the test structure

be26894

Bug fix

033de85

....

608ffe2

Bug fix

03b181e

Added mixed hostname tests using proxy

bdf3bfa

Removed initial exploratory tests

75abd64

Comment out Eclipse before merge

ddc8212

White space

c572931

Removed unused test resources

7f97316

Deleted .gitignore hiding local build artefacts

1edaf16

Removed eclipse plugin from build.gradle

4a14d47

pdowler requested changes Aug 5, 2025

View reviewed changes

Fixes for pull request review comments

75611fb

Zarquan requested a review from pdowler August 15, 2025 05:45

Fixing Javadoc issues

73eedce

pdowler reviewed Aug 19, 2025

View reviewed changes

Zarquan closed this Sep 25, 2025

Zarquan mentioned this pull request Sep 25, 2025

SKA-SP-5555 #219

Merged

Zarquan deleted the 20250804-SKAO-SP-5555 branch October 30, 2025 12:50

Conversation

Zarquan commented Aug 4, 2025

Uh oh!

pdowler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zarquan commented Aug 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zarquan commented Sep 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants