DM-48169: Add new components_json field to the jira_fields table. #28
Conversation
sebastian-aranda
force-pushed
the
tickets/DM-48169
branch
from
a437f9f to
c7a7dd7
Compare
There was a problem hiding this comment.
I didn't see where tests for "find messages" were. I'd like to see how the tests for the json component is done (so that I can see how the API for searching pieces will be done). But maybe there is NO search enabled for the component_json? But if that's the case then there would be no substitute for the stuff it replaces when deprecation happens.
| @@ -60,8 +60,14 @@ def create_message_table(metadata: sa.MetaData) -> sa.Table: | |||
| sa.Column("date_invalidated", saty.DateTime(), nullable=True), | |||
| sa.Column("parent_id", UUID(as_uuid=True), nullable=True), | |||
| # Added 2022-07-19 | |||
| # 'systems' field is deprecated and will be removed in v1.0.0. | |||
There was a problem hiding this comment.
I don't know any foolproof way to do deprecation for an API unless the API is always accessed via a paired client. Pairing client and API would be important if there were lots of non-Rubin users using the API. I assume that is NOT the case (if my assumption is wrong, maybe we should talk about what I've done in NOIRLab). But, it would be good to add an API endpiont that gives a semantic version. API documentation could warn that when the major component of the version increments, there have backwards incompatible changes. That would at least let API user know when an API change put their (API using) code in a dangerous place.
There was a problem hiding this comment.
Got it 👍 I'll work on adding a /version endpoint.
src/narrativelog/message.py
Outdated
| default_factory=dict, | ||
| title="JSON representation of systems and subsystems on the OBS jira project. " | ||
| "For a full list of valid keys please refer to: " | ||
| "https://rubinobs.atlassian.net/wiki/spaces/LSSTCOM/pages/53741849" |
There was a problem hiding this comment.
When I look at the documented linked to for "full list of valid keys", I don't see what those keys are. When I search that document for "key", the only two matches are "Keysight". I'm expecting to see something that clearly describes a dictionary (possibly with and example of the full dictionary).
There was a problem hiding this comment.
Got it 👍, I'll iterate over this too to make it more specific.
| "**This field is deprecated and will be removed in v1.0.0**. " | ||
| "Please use 'components_json' instead.", | ||
| ), | ||
| components_json: None |
There was a problem hiding this comment.
Maybe the API documentation is in another place. If this documentation that will show on swagger, there should be more detail here. Its not clear to me if you are allowing for search against the parts of this JSONB file. If you are allowing search against parts, it will take a special kind of search which means special kind of description for how to do it. When I've done something similar, I was able to make searches against the keys in the jsonb field to look like searches in the schema columns. To do so, I confined the jsonb field to one level (keys within in are like column names in the schema, and the values associated with they keys are column values. ) I did that because end users (anyone in the world) might access the API. There are more restrictions on your narrativelog API. Or maybe you are handling it somewhere else and I just haven't gotten to that part yet.
| components_path: None | ||
| | str = fastapi.Query( | ||
| default=None, | ||
| description="Components structure in JSON format." | ||
| "All messages with a 'components_json' field that " | ||
| "matches the specified structure are included." | ||
| "The structure represents the hierarchy of " | ||
| "systems and subsystems on the OBS jira project. " | ||
| "Current structure: `{'systems': ['system1', ..., 'systemN'], " | ||
| "'subsystems': ['subsystem1', ..., 'subsystemN'], " | ||
| "'components': ['component1', ..., 'componentN']}`. " | ||
| "E.g. `{'systems': ['AuxTel'], 'subsystems': ['ATCamera']}` will match" | ||
| " all messages that have 'AuxTel' as system and 'ATCamera' as subsystem.", |
There was a problem hiding this comment.
Ah, THIS is was I was expecting to see earlier.
But its not clear to me what the parameter value would look like for "component_path" search. Will the pieces be combined with AND? With OR? (I don't know where the implementation is for that kind of stuff with fastAPI. Hoping it uses ORM to combine instead of SQL.)
There was a problem hiding this comment.
Good catch! I was expecting the example to implicitly indicate how the search will work, i.e. all messages that have 'AuxTel' as system and 'ATCamera' as subsystem. meaning that we are combining key-values over AND. I'll iterate over this to provide something more similar to what we have before with "include any of" and "exclude any of" 👌.
There was a problem hiding this comment.
Approving based upon anticipated tweaks discussed with Sebastian via slack. Mainly: will add "version/" endpoint and will add description to https://usdf-rsp-dev.slac.stanford.edu/narrativelog/docs#/default/find_messages_messages_get to explain how to do query using the new component_json field.
sebastian-aranda
force-pushed
the
tickets/DM-48169
branch
9 times, most recently
from
0e538e1 to
36ad769
Compare
Also add deprecation advice and code comments for systems, subsystems, cscs, components, primary_software_components and primary_hardware_components.
No description provided.