ivoa-std / DataLink

DataLink standard (DAL)
3 stars 6 forks source link

added optional fields for links table to support A&A; resolves #33 #50

Closed pdowler closed 3 years ago

pdowler commented 3 years ago

This includes the minimal (true|false) usage of new optional columns. There are still several things to iron out:

I added the optional columns as a second table rather than adding a mandatory vs optional flag to the current table of fields. At a minimum the table placement and page folds are currently confusing. It would be nice if the tables were the same absolute width (LaTeX help?). If we add optional fields to the existing table and have only one, other text will need to be adapted.

For both fields, I am not sure that boolean (true|false) is quite enough but I found it hard to come up with a good use case for a third value:

I can't really envision any use cases where link_authorized needs a value other than true or false (or null for unknown aka cannot predict).

So - this is a draft pull request requiring additional work before completion.

msdemlei commented 3 years ago

Hi Pat,

On Tue, Nov 03, 2020 at 11:29:56AM -0800, Patrick Dowler wrote:

This includes the minimal (true|false) usage of new optional columns. There are still several things to iron out:

Hm -- since the column tables don't give types (and I think they should, in the rough system of integer/string/real), I don't think it's quite clear what true and false mean here -- are they supposed to be boolean columns? If so, do we care that that's going to make ADQL operation of in-database datalink tables (for which I could see use cases) difficult given our ADQL boolean woes?

Or are, as suggested by your musings about additional values in the PR text, "true" and "false" to be understood as text literals? [Incidentally, I'd favour that]. If so, I'd probably stress that somewhere and typographically write something like \verb|'true'| whenever referring to values in these columns.

I added the optional columns as a second table rather than adding a mandatory vs optional flag to the current table of fields. At a minimum the table placement and page folds are currently confusing.

There's an additional issue here: With the change, we have two levels of requiredness: The old columns are required to be present, but some of them may have null values. The new columns aren't required to be present, but it's at least conceivable that there may some some in the future that must be non-null if present.

In the tables, both concepts appear to be mapped to a column headed "required".

So, my vote would be: Have one table, and in there instead of the current "required" have three new columns: "required", "nullable", and "type".

If you agree with that and let me push into your repo, I could cover the typesetting side of that.

It would be nice if the tables were the same absolute width (LaTeX help?). If we add optional fields to the existing table and have

If you really want two tables and want to have constant columns widths, specify them explicitly rather than use the l code in the table preamble; the description column shows how to do it.

    -- Markus
pdowler commented 3 years ago

Yes, these two are text fields with a small set of values and I think I prefer a single table as well. I will augment the PR and then maybe ask for some latex help :-)

Having thought about it more, I am certain that link_auth requires 3 values: false, optional, true. The way we did TAP with multiple securityMethod(s) on the same accessURL is consistent with the work Brian has been doing on A&A and tokens and such confirms that this is the pattern.

  1. link_auth=false means the link only has (implied) anon securityMethod
  2. link_auth=optional means the link has anon and other securityMethod(s)
  3. link_auth=true means the link has 1+ securityMethod(s) and none are anon

Most CADC "capability/interface" are like 2, a few (mainly admin-ish things) are like 3, and a few things are like 1.

pdowler commented 3 years ago

I've modified the table to specify field required and value required. All datatypes are text except one (content_length is long) so I'm leaving that in the text only. The table is pretty wide and the description column is now highly hyphenated so I think we need to remove the descriptions from the table and make sure the text in following subsections includes it (it mostly does).

I will push what I have so you can see what it looks like.

Thoughts?

msdemlei commented 3 years ago

I'm fine with the content; I would still change quite a bit typographically (table formatting, using quotes to signify string literals, using typographic quotes in running text), but I can do that in a separate PR.

However, I can't add an approving review.

Todo for the ivoatex-github update: Give a crib for how to set up reviewers.