-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Imports into a group that is not defined as the default fail #19
Comments
The Issue?So I've done some digging and it looks like the import failure @juliomateoslangerak reported boils down to the fact that
This rules out my suggestion of adding a group argument to Smuggler's web import interface. Here's how to reproduce the issue using vanilla My set up
Plans o' mice and menUse Chronicle of a doomed importVersion check:
Log me in, Scotty!
Note it says "Current group: private".
...and try your luck with the import, note the
Oh, oh, what's that "Ignoring group since session key set"?! Spill the beans!
|
Just to add a bit more detail, if you acquire a session for the group you want to import with, then everything works smoothly:
Note this session's current group is the
and plonk it in:
and the file gets shovelled down OMERO's mouth. |
Where does that leave us? Given that Smuggler gets given a session key to do an import, it looks like the easiest way to work around the issue is not to do anything at all in Smuggler :-) In fact, it's probably easier to require import clients to pass along a suitable session key. In other words, if Insight (or any other client) wants to import into a dataset belonging to a group other than the user's default, it should get a session for that group. Incidentally, this is what
@jburel, @joshmoore you agree this is the easiest? Or is there a better way of doing this? On another note, why isn't the |
Did you try passing |
Just tried using the group ID but it makes no difference, I get the same error. It looks like the For the record, here's what I've done. As before, I logged in and grabbed my session key:
Then I tried importing
Kaboom! :-) |
@jburel, @joshmoore what should we do? I can see two options here:
Any other option you can think of? |
@jburel @joshmoore @dominikl it looks like this issue is related to: is that right? |
It's the same type of problem, but adding omero.group to the gateway won't fix your issue since import doesn't use the gateway.
@c0c0n3 : is the session shared between multiple processes? If so, -g can't do what it wants which is to change the group on the session since that would be a race condition. Essentially what you want is an application-wide setting, but that doesn't currently exist. (I don't imagine it would be too hard to add into |
@joshmoore thanks for clarifying, mucho appreciato!
Nope, in my tests above, the only process using the session was the OMERO |
Ah, ok. Then you should be able to use |
Awe! I should try that! But isn't the
Assuming the specified session isn't shared as you pointed out. |
I'm sure it can be debated, but I think the tension here is "What do you do if the user passed |
Yep, I see your point and agree with you for the login case. But in my opinion authorisation shouldn't be tied to the group the user logged in with. For example, say I'm a member of two groups: Instead for practical purposes, I'll try using the session service to change my group before importing, thank you so much for the suggestion!!! 👍 |
I'm not sure to get into the technical details. |
Just moving across this issue from the old Smuggler repo. Here's the issue as reported by @juliomateoslangerak: c0c0n3/ome-smuggler#16
Note. When resolved, we should close it in the Smuggler repo too.
It looks like we might be able to add the group option available in the OMERO CLI import command to the cli component in this repo and then make it available through the server component's import REST API as well as the soon-to-be-released Insight with integrated offline import functionality. This is doable as long as the OMERO Java import lib that the cli component relies on actually supports that group option too.
The text was updated successfully, but these errors were encountered: