preaction / Yancy

The Best Web Framework Deserves the Best Content Management System
http://preaction.me/yancy/
Other
54 stars 21 forks source link

bug: list controller doesn't use real schemas properties if no x-view properties are set #118

Closed mario-minati closed 3 years ago

mario-minati commented 4 years ago

The doc in Yancy::Help::Config / x-view says:

If no properties are given the "real" schema's one's are used.

But there is no code in Yancy::Controller::Yancy::list action that takes care of getting the "real" schema's properties.

The code could be fixed at https://github.com/preaction/Yancy/blob/master/lib/Yancy/Controller/Yancy.pm#L384 like this:

    my $schema = $c->yancy->schema( $schema_name )  ;
    my $real_schema_name = ( $schema->{'x-view'} || {} )->{schema} // $schema_name;
    my $props = $schema->{properties}
        || $c->yancy->schema( $real_schema_name )->{properties};
    my %param_filter = ();
preaction commented 4 years ago

I looked at this last night and was wondering if this could be better-fixed deeper in the code: The yancy.schema helper could be returning what the schema actually looks like, filling in the missing data that all of Yancy assumes is there (do a search through the code for { 'x-id-field' } // 'id' and you'll see what I mean). Or... the backend's schema method could do it, which would be better from a design perspective: The backend gives meaning to the JSON schema, so it should be able to do what it needs to do with it.

All that is to say I'm thinking about this, but I think that while I'm thinking about that I will merge your suggested patch so that you can get on with what you're doing :) I'm very interested to see where you're going with it: Stretching tech projects past what the original author has been doing with it is how the best features are invented :)

mario-minati commented 4 years ago

Your idea of extending the yancy.schema code to return an upgraded dataset seems to me a very good idea. Moving the code into the backend would require almost the same code in all Backend::* classes.

Currently I'm using a subclassed Yancy::Controller::Yancy anyway, so merging can wait on my behalf. :)

preaction commented 4 years ago

I'm leaning towards making Yancy::Backend a superclass to inherit from and making schema method return the full, modified if necessary, actual schema. I think this makes more sense from a design perspective: The Backend is the authority on what the data looks like, so it'd be perfectly fine for the Backend to control the entire JSON schema and forbid schema configuration entirely (the Yancy::Backend::Static backend technically does this: The main pages schema is static and the user is allowed to add fields to it).

mario-minati commented 4 years ago

This is a good idea, as it gives a lot more flexibility. Currently we are postprocessing the schema after Yancy setup in Mojolicious to inject the values according to the results of creating the schema from backend. This could be handled in a custom class.

preaction commented 3 years ago

It turns out Yancy went in a bit of a different direction: Yancy::Model. With a class behind it, you have complete control over how the schema is used. When the editor / OpenAPI modules are updated to work in terms of Yancy::Model::Schema objects, that will allow for the same functionality as the x-view schema directive, and more.