Skip to content
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

Scheduler command arg validation #6013

Open
hjoliver opened this issue Mar 11, 2024 · 5 comments · May be fixed by #6064 or #6112
Open

Scheduler command arg validation #6013

hjoliver opened this issue Mar 11, 2024 · 5 comments · May be fixed by #6064 or #6112
Assignees
Labels
could be better Not exactly a bug, but not ideal.
Milestone

Comments

@hjoliver
Copy link
Member

Follow-up #5658

The new cylc set command has relatively complicated validation requirements. During discussion there, @oliver-sanders suggested validating async scheduler commands server-side before queueing the command, instead of inside the queued command method itself. This enables validation feedback to the CLI and GUI.

I have a branch in progress, but it impacts other commands too, so it's better done as a quick-fire follow-up.

@hjoliver hjoliver added the could be better Not exactly a bug, but not ideal. label Mar 11, 2024
@hjoliver hjoliver added this to the cylc-8.3.0 milestone Mar 11, 2024
@hjoliver hjoliver self-assigned this Mar 11, 2024
@oliver-sanders
Copy link
Member

It is possible to validate a request and cause it to fail if needed, but only before the command is queued.

Cylc used to have a layer to do this, it looked for a "proxy" command:

method = getattr(self, command, None)
if method is not None:
return method(**kwargs)

If one was provided, it would call it rather than looking for a scheduler method:

def put_ext_trigger(
self,
message,
id # noqa: A002 (graphql interface)
):
"""Server-side external event trigger interface.
Args:
message (str): The external trigger message.
id (str): The unique trigger ID.
Returns:
tuple: (outcome, message)
outcome (bool)
True if command successfully queued.
message (str)
Information about outcome.
"""
self.schd.ext_trigger_queue.put((message, id))
return (True, 'Event queued')

This was the vestigial remains of the old HTTP server where each command had a "proxy" in the server code:

@cherrypy.expose
@cherrypy.tools.json_out()
def get_latest_state(self, full_mode=False):
"""Return latest suite state (suitable for a GUI update)."""
client_info = self._check_access_priv_and_report(PRIV_FULL_READ)
full_mode = self._literal_eval('full_mode', full_mode)
return self.schd.info_get_latest_state(client_info, full_mode)

The need for this layer dropped away thanks to the better type support and validation features of GraphQL so the "proxy" commands were removed on by one.

The last remains of this interface were removed with #5658 as they were no longer used. To add validation back in, we'll need something like this layer. We could potentially re-implement this with a little syntactic sugar e.g:

def validate(validate_fcn, command_fcn):
    command_fcn.validate = validate_fcn
    return command_fcn

def validate_set(args):
    if ...:
        raise InputError('some argument must be ....')

class Scheduler:
    # ...

    @validate(validate_set)
    @def command set(...):
        ...


class Resolvers:
    # ...

        meth = getattr(schd, command)
        if meth.validate:
            try:
                meth.validate(args)
            except InputError as exc:
                 # TODO: return error
        schd.command_queue.put((meth, args))

@hjoliver
Copy link
Member Author

Yes that's the approach I'm taking.

@hjoliver
Copy link
Member Author

A detail:

I also spotted that the --pre option doesn't appear to work with the --wait option, which makes sense.
Another one for validation?

@hjoliver
Copy link
Member Author

Working on this today, PR tomorrow...

@ColemanTom ColemanTom linked a pull request Apr 11, 2024 that will close this issue
8 tasks
@dwsutherland
Copy link
Member

Ha! I haven't seen that httpserver.py in a while..

@oliver-sanders oliver-sanders linked a pull request May 30, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
could be better Not exactly a bug, but not ideal.
Projects
None yet
3 participants