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

1.0.0-rc.1 implementation issues #260

Closed
12 tasks done
m-mohr opened this issue Feb 12, 2020 · 1 comment
Closed
12 tasks done

1.0.0-rc.1 implementation issues #260

m-mohr opened this issue Feb 12, 2020 · 1 comment

Comments

@m-mohr
Copy link
Member

m-mohr commented Feb 12, 2020

As I'm currently implementing 1.0.0-rc.1 for GEE, here's a list of what came up:

General

Related to process graphs (user-defined processes)

  • A. Sending process graphs to /services, /jobs and /result is lacking the default values from the parameters that are available in /process_graphs. This may lead to issues during execution when user hasn't specified the parameters. See PR Pass process metadata with data processing requests #262
  • B. GET /process_graphs: Field id is not required for the processes. Fixed in 05bc778
  • C. POST /process_graphs is not following REST. It should be PUT /process_graphs/{process_graph_id} as we actually set the id ourself. From docs it's also not 100% clear whether the process graph id (e.g. in the path) is the same as a process id. See PR PUT /process_graphs/{id} #261
  • D. PATCH /process_graphs/{process_graph_id} allows to change the id, but the behavior afterwards is unclear. In theory the path must change. I tend to forbid changing the ID. See PR PUT /process_graphs/{id} #261
  • E. Usually in responses like GET /jobs/{job_id} or GET /services/{service_id} most (all?) optional fields can be set to null, which makes implementation a bit easier. For the process graphs at /process_graphs none of the fields can be set to null and so they must actually be missing from the response if no data is available. That's more difficult to implement, but aligned with /processes. Should we introduce nullable for some/all fields in /process_graphs and/or /processes? For more information see issue Make optional fields nullable in process-related endpoints? #264.
  • F. Related to E: As you can't set null in PATCH /process_graphs/{process_graph_id} you can't easily "reset" fields. For example, you can only remove the description by sending an empty string. For parameters you'd need to send am empty object, but that means "no parameters" where as not having parameters at all means the parameters are simply unknown/unspecified, but there may be some. See PR PUT /process_graphs/{id} #261

We may head for a quick 1.0.0-rc.2 as I guess there wasn't much work put into 1.0.0-rc.1 implementations yet.

@m-mohr m-mohr added this to the v1.0-final milestone Feb 12, 2020
@m-mohr m-mohr pinned this issue Feb 12, 2020
@m-mohr m-mohr changed the title Impementation issues/thoughts 1.0 implementation issues Feb 12, 2020
@m-mohr m-mohr changed the title 1.0 implementation issues 1.0.0-rc.1 implementation issues Feb 12, 2020
@m-mohr
Copy link
Member Author

m-mohr commented Feb 12, 2020

Ideas:

  1. For A there are two options:
  2. For C, D and F:
    • POST /process_graphs and PATCH /process_graphs/:id with a single endpoint PUT /process_graphs/:id. id can't be changed as it's part of the path. See PR PUT /process_graphs/{id} #261
  3. For E:
    • Allow null in optional fields in /process_graphs and /processes.
    • Leave as is and deal with the additional implementation effort.

@jdries @soxofaan @lforesta @aljacob @kempenep @flahn

m-mohr added a commit that referenced this issue Feb 12, 2020
…raph_id}` with `PUT /process_graphs/{process_graph_id}`. #260
m-mohr added a commit that referenced this issue Feb 12, 2020
…raph_id}` with `PUT /process_graphs/{process_graph_id}`. #260
@m-mohr m-mohr modified the milestones: v1.0-final, 1.0.0-rc.2 Feb 12, 2020
m-mohr added a commit that referenced this issue Feb 12, 2020
…raph_id}` with `PUT /process_graphs/{process_graph_id}`. #260
m-mohr added a commit that referenced this issue Feb 12, 2020
… (`/result`) the property `process_graph` got replaced by `process`. It contains a process graph and optionally all process metadata. #260
@m-mohr m-mohr closed this as completed Feb 20, 2020
@m-mohr m-mohr unpinned this issue Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant