Skip to content

Conversation

@SilviaSWR
Copy link
Contributor

Description:

This PR improves compatibility of the vos library with VOSpace servers that return multiple endpoint URLs.

Key Fixes:

Updated mkdir and move methods to handle cases where get_node_url returns a list of URLs.
These methods now use only the first URL, avoiding failures when interacting with HTTP-based VOSpace services.

Impact:

Prevents errors when calling mkdir or move on non-CADC VOSpace servers.
Improves reliability and cross-server support in the vos client.

Testing:

Code was tested against a VOSpace server returning multiple URLs. Operations complete without error.

Issue:
#233

@andamian
Copy link
Contributor

andamian commented Jul 9, 2025

Thank you for your submission. Can you provide more details about the multiple endpoints? Is that for http/https endpoints to the same resource? How are they advertised in the service capabilities. If that's the case, I would like to double check to make sure that the spec supports it.

@SilviaSWR
Copy link
Contributor Author

SilviaSWR commented Jul 10, 2025

Thank you for your question — here's a clarification:

This issue is not caused by multiple endpoint URLs advertised by the VOSpace service. Instead, it's due to the internal logic in the vos client itself, specifically in the get_node_url method.

Root Cause:

When the registered VOSpace service uses an HTTP or HTTPS URI scheme in the config file (i.e. a non-CADC setup), the get_node_url method returns a list containing a single URI rather than a string.

# Inside vos.py
if parts.scheme.startswith('http'):
    return [uri]  # This always returns a list

As a result, methods like mkdir and move pass this list into self.get_session(uri), which expects a string, not a list. This leads to a runtime error, even though the list only contains one valid endpoint.

How to Reproduce:

  1. Configure a VOSpace server in .vospace/config with an HTTP prefix, for example:
[vos]
resourceID = http://myserver.com http
  1. Run a vos mkdir or vos move using that http: prefix.
  2. It crashes when trying to establish a session, because uri is a list.

Proposed Fix:

In mkdir, move, and any similar methods, check if the returned uri is a list and, if so, use the first element.

This safely handles the internal inconsistency without affecting behavior for CADC-style: URIs, which continue to return strings.

@andamian
Copy link
Contributor

Disclaimer - the vos tools are heavily tuned to work with the CADC services so it's very likely that there might be problems when trying to use them with other services but we would like to start supporting this. I'm not totally sure your fix works since the config file is ~/.config/vos/vos-config.

If you are trying to use this in SRCNet, it might work with a bit of setup. If you can provide mode details about your use case, I can try to assist you. Where is the VOSpace service running? Does it have a reg entry?

@SilviaSWR
Copy link
Contributor Author

Thank you again for the follow-up! Here's some additional context about the use case:

VOSpace Service Configuration

I'm using the vos client against a VOSpace service we’re currently developing at PIC. The server is in the final stages of development but is not yet public. It has been implemented to follow the VOSpace standard, and while we can already access it using curl (as shown in the spec examples), we would like it to also be compatible with the vos client.

This service is not part of CADC, and it will be available using an HTTP(S) URI. It does not use IVOA registry integration, so there is no ivo:// identifier and no registry entry.

Custom Client Configuration

To test and configure our VOSpace service, I debugged the client code and found that it's possible to specify an alternative config file path, using the following logic in vos/vos/vosconfig.py:

_CONFIG_PATH = os.path.expanduser("~") + '/.config/vos/vos-config'
if os.getenv('VOSPACE_CONFIG_FILE', None):
    _CONFIG_PATH = os.getenv('VOSPACE_CONFIG_FILE')

So I set a custom config file for our server like this:

import os

# Set the environment variable
os.environ['VOSPACE_CONFIG_FILE'] = './data/vos-config'

With the following contents in ./data/vos-config:

[vos]
# List of VOSpace services known to vos, one entry per line:
# resourceID = <resourceID> [<prefix> (default: vos)]
resourceID = http://localhost:8800/vospace http

Goal and Collaboration Opportunity

Right now, we are aiming for basic VOSpace service compatibility.
We believe this is a good opportunity to improve interoperability between the vos client and non-CADC VOSpace implementations, and we’d be happy to contribute in that direction.

###Tested Operations
We’ve tested the vos client with our VOSpace server using the following operations:

import os
import vos

os.environ['VOSPACE_CONFIG_FILE'] = os.environ["HOME"] + '/data/vos-config'
client = vos.Client()

client.listdir('http://localhost:8800/vospace/nodes/my_dir')
client.delete('http://localhost:8800/vospace/nodes/my_dir')
client.mkdir('http://localhost:8800/vospace/nodes/my_dir')

# Download file:
client.copy('http://localhost:8800/vospace/nodes/file.txt', 'user/my_dir/file.txt')

# Upload file:
client.copy('user/my_dir/file.txt', 'http://localhost:8800/vospace/nodes/my_dir/file.txt')

# Move file:
client.move('http://localhost:8800/vospace/nodes/my_dir/file.txt', 'http://localhost:8800/vospace/nodes/my_dir/file1.txt')

# Get node metadata:
client.get_node('http://localhost:8800/vospace/nodes/my_dir/file1.txt')

All of these operations work correctly after applying the changes proposed in the following PRs:

The get_session method is not designed to receive a list of URIs, only a single URI.
While most client methods already ensure this (e.g., using .pop()), in some cases like copy, mkdir, or delete, this check is missing — and the code directly uses the output of get_node_url, which (for HTTP URIs) is always a list with a single item.

I'm glad to share a minimal test setup or assist with any further steps to support non-CADC compatibility. Thanks again for considering this contribution!

@codecov
Copy link

codecov bot commented Jul 15, 2025

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@78b7cc3). Learn more about missing BASE report.

Files with missing lines Patch % Lines
vos/vos/vos.py 66.66% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #234   +/-   ##
=======================================
  Coverage        ?   59.78%           
=======================================
  Files           ?       21           
  Lines           ?     2566           
  Branches        ?        0           
=======================================
  Hits            ?     1534           
  Misses          ?     1032           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@andamian
Copy link
Contributor

Sorry, I was off for a couple of days and couldn't respond earlier.

I appreciate the detailed explanation. This is clearly a bug. I don't like the fix, but that get_node_url should have been more consistent on the returned type. Since it fixes your case, I will accept it because it doesn't change the API so it's backwards compatible.

A few points to consider:

  • The custom config file is not necessary. The default ~/.config/vos/vos-config works as well. The entries in there however are not what they were meant for. The fact that it works it's just luck (or reverse engineering on your part)
  • If you change your service to provide a Content-Disposition header, then it can deduce the name of the file and this would work as well client.copy('http://localhost:8800/vospace/nodes/file.txt', 'user/my_dir/')
  • If your service also returns size and md5 headers, the copy will avoid copying files that already exist at the destination

Thank you for your contribution. I can merge as it is and I can craft a release too unless you are planning more testing. Again, this is old code, tuned to the CADC VOSpace services (and quite popular with its users). I've been planing however to re-implement this in PyVO (see astropy/pyvo#528) so if you are interested I would welcome collaborators with other VOService implementations.

@SilviaSWR
Copy link
Contributor Author

Thanks for the follow-up. From my side, you can go ahead with the merge, as I’ve completed the necessary testing. I’d appreciate it if you could go ahead with a release as well.

@andamian andamian merged commit 53b9f7d into opencadc:main Jul 16, 2025
11 checks passed
@andamian
Copy link
Contributor

3.6.3 is published. Thanks for your contribution @SilviaSWR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants