-
Notifications
You must be signed in to change notification settings - Fork 9
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
feat: add api to get stream state #586
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, looks good. A couple small comments and looks like there's a format issue to resolve but nothing major from me.
api/ceramic.yaml
Outdated
description: Metadata of the stream | ||
data: | ||
type: object | ||
description: State of the stream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"State" feels overloaded here.
"content" of the stream?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its not the content, its both the metadata and the data (previously called content). Maybe we should call this state, metadata, content?
The state is currently defined as:
struct State{
metadata: Object,
data: Object,
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, the problem is this whole structure is called StreamState
. Having a field called state
inside a struct called StreamState
feels confusing to me. Now when we hear the word "state" we don't know if we mean the inner field or the outer structure.
Its not the content, its both the metadata and the data
I think this isn't true, since there is already a different metadata field at the top level here. So having metadata
and data
at the top level avoids the need to expose anything called "state" to the user at all.
I don't love that internally we call the pair of metadata and data "state", but at least that's hidden as an internal implementation detail. If we can keep that concept out of the public API that might be good enough for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the confusion, let me see if I can make the table structure mirror the flattened structure of this StreamState object more closely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The trouble here is keeping this API generic to stream types. Metadata is a property of the MID stream type. That is causing the problems here. I say we leave metadata and data as opaque to this API
How does this look as a definition of the StreamState:
StreamState:
title: State of a Ceramic stream
description: The state of a Ceramic stream as defined by the stream type aggregation and conflict resolution rules.
type: object
required:
- id
- event_cid
- controller
- dimensions
- data
properties:
id:
type: string
description: Multibase encoding of the stream id
event_cid:
type: string
description: CID of the event that produced this state
controller:
type: string
description: Controller of the stream
dimensions:
type: object
description: Dimensions of the stream, each value is multibase encoded.
data:
type: object
description: Data of the stream
Then data
for MID streams will be an object of
{
metadata: ...,
content: ...,
}
Thoughts?
This way we do not overload names and uses of the existing clients will understand that content
is the same as the original meaning of content.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that sounds good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stbrody Ok adjusted to match the now consistent names and structure defined in the pipeline/DESIGN.md doc.
5deb304
to
c98ffc6
Compare
With this change it is now possible to get the current state of a stream via the HTTP JSON API.
c98ffc6
to
82eb5c9
Compare
With this change it is now possible to get the current state of a stream via the HTTP JSON API.
Fixes #581