LD4P / sinopia_server

[Deprecated - switching to MongoDB] Sinopia Back-end CRUD Service. LDP-inspired, HTTP Server taking JSON-LD resources & administrative metadata.
Apache License 2.0
1 stars 1 forks source link

Group and resource creation #66

Closed jmartin-sul closed 5 years ago

jmartin-sul commented 5 years ago

closes #35 (robustifying can be a follow on ticket, per TODO in code)

connects to #38 (in that i believe resource templates can be pushed into trellis the same way as profiles, since they're also non-RDF JSON blobs? so this should illustrate the use case in #38 also?) connects to #29 connects to #41

TODO:

happy to remove the TODOs from code if people would prefer, i tend not to leave those in, but things are pretty messy and in flux, so i'm also fine with a "go back and fix/ticket TODOs" ticket for the end of the work cycle. whatever people would prefer.

ndushay commented 5 years ago

My opinion on autogen stuff: let's not muck about with it. The html encoding can be ignored at this time (it might be a setting somewhere, but meh). The specific examples ... would be helpful, but fine if documented elsewhere. The indication of optional for a required param -- that seems worth spending 20 min on.

jmartin-sul commented 5 years ago

@mjgiarlo said

Left some questions on the PR, mostly about Link headers. Also seeing some HTML-escaped characters in the markdown and am not sure if those are intentional/required or just there for fun. 😄

for fun!

but yeah, hopefully the link header stuff is explained in a way that makes sense in the inline replies. i wasn't exactly sure what to do, since the auto-gen code didn't quite do what the swagger spec was able to express, and i'm of course always very hesitant to modify auto-gen code. but, considering my unfamiliarity with this sort of JS programming, my unfamiliarity with LDP, and the short time frame, my gut feeling is actually that holding my nose and making minimal modifications to decent auto-gen boilerplate and comparing to known working curl exampls over the wire to see how to get a working SDK is the way to go. totally happy to consider any other options (and in the case of the link header overrides, there's a slightly-more-cumbersome-for-caller way to get out of mucking w/ autogen JS). i figure even if we ultimately decide that we don't like this particular auto-generated code or swagger as a spec tool in general, we can just take what works out of this and discard the rest and build from there.

Further question: can you say more about the renaming, like from SinopiaBaseContainer to SinopiaBasicContainer? I ask because I'm not 💯 sure on the original intention vs. what has changed that motivates this work.

so, to start, my recollection (from discussions with @jermnelson and christina in late 2018) is that all of our RDF containers will be basic containers. i suspect this might even be captured in a design doc or ticket or something, but i'd have to look around. (jeremy, correct me if i'm wrong)

what i realized in practice was that the generic LDP container structures defined in the swagger spec by christina map well (AFAICT from what i know) to the semantic concepts of LDP. but they don't map to the actual field names we want to push across in our specific initial sinopia use cases, and so the JS code that gets generated doesn't end up being able to easily push across what we want.

so, as a first pass, i created SinopiaBaseContainer, a model that did what we want for the use case of the base container update. what i realized when going to implement the group basic container structure was that it looked exactly the same as the base container structure: https://github.com/LD4P/sinopia_server/blob/master/fixtures/base-container.jsonld https://github.com/LD4P/sinopia_server/blob/master/fixtures/group_defs/profiles.jsonld

since they're both actually just LDP basic containers, it seemed reasonable to call that our sinopia specific LDP basic container structure, hence, SinopiaBasicContainer and its context object: https://github.com/LD4P/sinopia_server/blob/group_and_resource_creation/swagger.yaml#L1146-L1177

i was hesitant to explain all that in comments or PR description, because y'all seem more familiar with LDP concepts than me, and i always get dinged for writing novels in my comments =)

@ndushay said

My opinion on autogen stuff: let's not muck about with it.

so if we did want to avoid doing that in the couple spots where i did muck about with auto-gen code, we could just do what i did for the content type header, and force the caller to specify it. no strong preference on that for me, was sort of a toss up, i just ended up falling the other way in what i implemented.

jmartin-sul commented 5 years ago

@mjgiarlo, one other bit of background on the overall design of sinopia server: my recollection is that it's meant to be "LDP-ish" or "LDP inspired", but that it isn't necessarily a strictly LDP conformant server (but sort of a subset so far based on the swagger spec? our API constrains the sinopia client(s) to a subset of what's possible in real LDP?).

@jermnelson correct me if i'm misremembering that or if that's changed. i recall seeing that in some design docs, and recall christina saying it.

the trellis backend is of course an actual LDP server.

jermnelson commented 5 years ago

@mjgiarlo, one other bit of background on the overall design of sinopia server: my recollection is that it's meant to be "LDP-ish" or "LDP inspired", but that it isn't necessarily a strictly LDP conformant server (but sort of a subset so far based on the swagger spec? our API constrains the sinopia client(s) to a subset of what's possible in real LDP?).

I think so, it was my recollection that the client supports our use cases but is a subset of the LDP spec.

@jermnelson correct me if i'm misremembering that or if that's changed. i recall seeing that in some design docs, and recall christina saying it.

That is what I recall as well. the trellis backend is of course an actual LDP server.

mjgiarlo commented 5 years ago

@jmartin-sul Thanks for the explanation!

jmartin-sul commented 5 years ago

@ndushay, added ticket links to TODOs