livingsocial / swagger_yard

Swagger-UI compliant JSON generated from YARD. For RESTful Rails apps.
MIT License
51 stars 28 forks source link

Type parsing #40

Closed nicksieger closed 8 years ago

nicksieger commented 8 years ago

Inspired by @bfad's PR #39, I went ahead and looked into what it would take to implement a proper type parser.

bfad commented 8 years ago

This looks really good and exactly what I was going for. One feature suggestion: Do you think we could add in a description of the property? I'm not sure how feasible that will be as it may be difficult to delineate when the description ends and the next property begins. Here's what I mean: Right now properties are comma-separated, and that means the descriptions couldn't have commas in them. We could encase each property in it's own set of angled brackets, but then comments couldn't use the greater-than sign. I haven't looked, but was wondering if maybe parslet has the ability to work around this or if you could think of another solution.

timbogit commented 8 years ago

@nicksieger : 💯 on introducing an actual parser and factoring this logic out of type.rb, thanks so much for doing this!

@bfad : Can you add some suggestions to how adding descriptions to objects would look like? I am generally worried that adding all possible fields right in the top-level property descript might become a little unwieldy, especially when you allow for this to 'recurse all the way down' to describing the properties of the object, etc.. I.e., I would be worried about Swagger YARD docs like:

@parameter foobar_array [array<object<"It's a FooBar", foo:string("This is the foo part"),bar:integer<int32>("This is the bar part")>>]  FooBar array for ....

IMHO, it would be better to nudge the user towards defining dedicated models where the properties can then be documented specifically and in a more readable fashion.

That said, if other maintainers do nat share this concern, or if there is a better / more succinct and readable way of defining objects with all their attributes directly than what I have 'pseudo-code' above, then I'd love to see it.

nicksieger commented 8 years ago

@timbogit I agree. I think adding inline descriptions to the type parser is going too far. As it is, having a full, balanced parser for types possibly enables structures that can be more deeply nested than is practical. If you want to add descriptions for nested objects, they should be defined as their own models. Is there any reason why that is unwieldy?

bfad commented 8 years ago

@timbogit I think the unwieldiness of the solution would nudge users to create their own models. Here's an example of what I was thinking (which does currently work with this PR without the descriptions):

@error_message 400 [object<errors:array<
  object<
    code: enum<ValidationError>,
    title: string(A description of the attribute and it's problem.),
    source: string(A JSON pointer to the offending attribute in the submitted JSON data.)
  >>>] A group of validation errors

I don't know if you've looked at the JSON API specification, but it would be really difficult to use SwaggerYard to document it. I've put their sample response to getting a list of articles below. Granted most requests won't return all that complexity, but I still don't see how to document this nicely in SwaggerYard — it looks like I would have to create a lot of fake/empty classes just to be able to have nested models for SwaggerYard to reference.

{
  "links": {
    "self": "http://example.com/articles",
    "next": "http://example.com/articles?page[offset]=2",
    "last": "http://example.com/articles?page[offset]=10"
  },
  "data": [{
    "type": "articles",
    "id": "1",
    "attributes": {
      "title": "JSON API paints my bikeshed!"
    },
    "relationships": {
      "author": {
        "links": {
          "self": "http://example.com/articles/1/relationships/author",
          "related": "http://example.com/articles/1/author"
        },
        "data": { "type": "people", "id": "9" }
      },
      "comments": {
        "links": {
          "self": "http://example.com/articles/1/relationships/comments",
          "related": "http://example.com/articles/1/comments"
        },
        "data": [
          { "type": "comments", "id": "5" },
          { "type": "comments", "id": "12" }
        ]
      }
    },
    "links": {
      "self": "http://example.com/articles/1"
    }
  }],
  "included": [{
    "type": "people",
    "id": "9",
    "attributes": {
      "first-name": "Dan",
      "last-name": "Gebhardt",
      "twitter": "dgeb"
    },
    "links": {
      "self": "http://example.com/people/9"
    }
  }, {
    "type": "comments",
    "id": "5",
    "attributes": {
      "body": "First!"
    },
    "relationships": {
      "author": {
        "data": { "type": "people", "id": "2" }
      }
    },
    "links": {
      "self": "http://example.com/comments/5"
    }
  }, {
    "type": "comments",
    "id": "12",
    "attributes": {
      "body": "I like XML better"
    },
    "relationships": {
      "author": {
        "data": { "type": "people", "id": "9" }
      }
    },
    "links": {
      "self": "http://example.com/comments/12"
    }
  }]
}
nicksieger commented 8 years ago

I took a cursory look at JSON API. Could you model it like this?

# @model JsonApiLinks
# @property self      [string]  URI
# @property prev      [string]  URI
# @property next      [string]  URI
# @property last      [string]  URI
# @property related   [string]  URI
class JsonApiLinks; end

# @model JsonApiLinksModule
# @property links     [JsonApiLinks]
class JsonApiLinksModule; end

# @model JsonApiResource
# @property type  [string]
# @property id    [string]
# @property attributes     [object]
# @property relationships  [object<JsonApiRelationship>]
# @inherits JsonApiLinksModule
class JsonApiResource; end

# @model JsonApiRelationship
# @property data [array<ApiJsonResource>]
# @inherits JsonApiLinksModule
class JsonApiRelationship; end

# @model JsonApiResponse
# @property data      [array<JsonApiResource>]
# @property included  [array<JsonApiResource>]
# @inherits JsonApiLinksModule
class JsonApiResponse; end
bfad commented 8 years ago

Probably something like that, but that was my point. It seems kind of silly to have to have a bunch of fake/empty classes just to be able to properly document a JSON schema I'm either expecting or returning. (Note: the attributes I'm most interested in documenting with descriptions are those in JsonApiResource's attributes which you have listed as @property attributes [object], so there would need to be another layer of nesting there.)

timbogit commented 8 years ago

FWIW, I personally find @nicksieger 's explicit and inheritance-based documentation much more appealing, readable, reusable, and maintainable than inlined object type descriptions.

But I also see @bfad 's point regarding the unfortunate need to create actual Ruby classes. This is a known limitation with swagger-yard (due to its use of YARD), where each @model tag needs to be tied to a Ruby class.

All things considered, I would still prefer @nicksieger 's approach, even if it is admittedly more verbose.

bfad commented 8 years ago

I agree that this approach is clearer if slightly dirtier with the empty classes. I just wish I could some how either create multiple models inline or do nested properties like the apipie DSL allows for. Both of those desires are probably out of scope for this PR, and I'm happy with the new parser. Thanks!

timbogit commented 8 years ago

@nicksieger I merged this into master, and opened #41 for Changelog, README, and version bump.