Add a POST /nodes/{name}/validate/ endpoint for revalidating already existing nodes by name#619
Add a POST /nodes/{name}/validate/ endpoint for revalidating already existing nodes by name#619samredai merged 1 commit intoDataJunction:mainfrom samredai:revalidate
POST /nodes/{name}/validate/ endpoint for revalidating already existing nodes by name#619Conversation
✅ Deploy Preview for thriving-cassata-78ae72 canceled.
|
samredai
changed the title
POST /nodes/{name}/validate/ endpoint for revalidating already existing nodes by name
shangyian
left a comment
shangyian
left a comment
There was a problem hiding this comment.
Looks useful!
Once we've ironed out all the bugs, do you think we'll still want this endpoint though?
agorajek
left a comment
agorajek
left a comment
There was a problem hiding this comment.
Agree with @shangyian in that I think this endpoint should be internal only. Meaning if a user ever need to use it then we have a bigger problem that should be addressed.
2 questions in-line.
| node = get_node_by_name(session, name) | ||
| current_node_revision = node.current | ||
| if current_node_revision.type == NodeType.SOURCE: | ||
| current_node_revision.status = NodeStatus.VALID |
There was a problem hiding this comment.
This is dangerous, if the source node does not match the underlying table anymore. Right?
There was a problem hiding this comment.
You're right, I hadn't considered that reflection could be setting a source node as invalid. Good catch!
There was a problem hiding this comment.
@agorajek thinking about this a bit more, since we're going to leave this as an admin endpoint, is it valuable to leave this here as an escape hatch for when a reflection service is acting up?
| else: | ||
| status = NodeStatus.VALID | ||
|
|
||
| current_node_revision.status = status |
There was a problem hiding this comment.
What if current_node_revision.status already equals to status. Would we need to create a new revision?
There was a problem hiding this comment.
This doesn't create a new revision, it just updates the system status value on the existing revision. But good point that there's no reason to even do a db commit at all so I'll fix this.
There was a problem hiding this comment.
Updated to check if current_node_revision.status != status:
|
Makes sense! I'll update this PR to remove the UI changes then and we can just call the endpoint when needed. EDIT: Done |
3 participants
Summary
Currently DJ attempts to validate nodes as any live change is being made to the system. When things are missed or preconditions are fulfilled after the fact, this PR provides a mechanism to request the server to revalidate an existing node, requiring that you simply provide the node name. Over time, we should catch these scenarios more and more so that system status is more accurately captured in realtime and revalidation is never required.
Here's a video example of how this was added to the UI. I manually set nodes in the metadata that I knew to be valid as invalid, and clicking the icon revalidated them and updated the status. If they were invalid, the status icon would have stayed as a red x.
revalidate.mov
Test Plan
docker compose --profile demo upand tested it in the UI. Also tested curl commands directly to the dj server.make checkpassesmake testshows 100% unit test coverageDeployment Plan