[PATCH] add xslt_stylesheet_param directive

Maxim Dounin mdounin at mdounin.ru
Tue Jan 31 01:52:23 UTC 2012


Hello!

On Tue, Jan 24, 2012 at 11:02:13PM +0100, SamB wrote:

> Hi,
>    next try...
> 
>    Attached patch adds xslt_stylesheet_param directive processing.
>    This is equivalent of xslt parameter passed to xslt_stylesheet, but this
> works independently and globally.
>    xslt config should be a bit simpler because 'common' xslt variables
> don't need to be repeated for each xslt_stylesheet.
> 
>    Comments/suggestion are welcomed.

You may have better lock pointing to a previous incarnation of the 
patch and outlining changes made.  (And please post in plain text, 
not html.)

It was previously submitted here:

http://mailman.nginx.org/pipermail/nginx-devel/2012-January/001669.html

And previous review was here:

http://mailman.nginx.org/pipermail/nginx-devel/2012-January/001670.html


> diff -ur nginx-1.1.13/src/http/modules/ngx_http_xslt_filter_module.c nginx-1.1.13-new/src/http/modules/ngx_http_xslt_filter_module.c
> --- nginx-1.1.13/src/http/modules/ngx_http_xslt_filter_module.c	2010-07-12 14:52:01.000000000 +0200
> +++ nginx-1.1.13-new/src/http/modules/ngx_http_xslt_filter_module.c	2012-01-24 22:33:24.000000000 +0100
> @@ -48,6 +48,7 @@
>      ngx_array_t          sheets;       /* ngx_http_xslt_sheet_t */
>      ngx_hash_t           types;
>      ngx_array_t         *types_keys;
> +    ngx_array_t          params;       /* ngx_http_complex_value_t */
>  } ngx_http_xslt_filter_loc_conf_t;
>  
>  
> @@ -84,12 +85,16 @@
>      void *conf);
>  static char *ngx_http_xslt_stylesheet(ngx_conf_t *cf, ngx_command_t *cmd,
>      void *conf);
> +static char *ngx_http_xslt_stylesheet_param(ngx_conf_t *cf, ngx_command_t *cmd,
> +    void *conf);
>  static void ngx_http_xslt_cleanup_dtd(void *data);
>  static void ngx_http_xslt_cleanup_stylesheet(void *data);
>  static void *ngx_http_xslt_filter_create_main_conf(ngx_conf_t *cf);
>  static void *ngx_http_xslt_filter_create_conf(ngx_conf_t *cf);
> +static char *ngx_http_xslt_filter_merge_params(const ngx_array_t *src, ngx_array_t *dest);
>  static char *ngx_http_xslt_filter_merge_conf(ngx_conf_t *cf, void *parent,
>      void *child);
> +static ngx_int_t ngx_http_xslt_filter_preinit(ngx_conf_t *cf);
>  static ngx_int_t ngx_http_xslt_filter_init(ngx_conf_t *cf);
>  static void ngx_http_xslt_filter_exit(ngx_cycle_t *cycle);
>  
> @@ -116,6 +121,13 @@
>        0,
>        NULL },
>  
> +    { ngx_string("xslt_stylesheet_param"),
> +      NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1,
> +      ngx_http_xslt_stylesheet_param,
> +      NGX_HTTP_LOC_CONF_OFFSET,
> +      0,
> +      NULL },

I tend to think that "xslt_stylesheet_param" is too long.  
Something like "xslt_param" whould be better.

> +
>      { ngx_string("xslt_types"),
>        NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_1MORE,
>        ngx_http_types_slot,
> @@ -128,7 +140,7 @@
>  
>  
>  static ngx_http_module_t  ngx_http_xslt_filter_module_ctx = {
> -    NULL,                                  /* preconfiguration */
> +    ngx_http_xslt_filter_preinit,          /* preconfiguration */

The "...preinit" change is unrelated.  If you want it to happen - 
please submit it seprately.  (I think it's noop though.)

>      ngx_http_xslt_filter_init,             /* postconfiguration */
>  
>      ngx_http_xslt_filter_create_main_conf, /* create main configuration */
> @@ -865,6 +877,46 @@
>  }
>  
>  
> +static char *
> +ngx_http_xslt_stylesheet_param(ngx_conf_t *cf, ngx_command_t *cmd, void *conf)
> +{
> +    ngx_http_xslt_filter_loc_conf_t  *xlcf = conf;
> +
> +    ngx_http_complex_value_t         *param;
> +    ngx_http_compile_complex_value_t  ccv;
> +    ngx_str_t                        *value;
> +
> +    value = cf->args->elts;
> +
> +    /* alloc array */
> +    if (xlcf->params.elts == NULL
> +    	&& ngx_array_init(&xlcf->params, cf->pool, 1, sizeof(ngx_http_complex_value_t))
> +        != NGX_OK)
> +    {
> +        return NGX_CONF_ERROR;
> +    }

No lines longer than 80 chars, please.  And alignment isn't 
styl-complaint either.  In this particular case, please just use 
another nested if, much like in ngx_http_xslt_stylesheet():

    if (xlcf->sheets.elts == NULL) {
        if (ngx_array_init(&xlcf->sheets, cf->pool, 1,
                           sizeof(ngx_http_xslt_sheet_t))
            != NGX_OK)
        {
            return NGX_CONF_ERROR;
        }
    }

> +
> +    param = ngx_array_push(&xlcf->params);
> +    if (param == NULL) {
> +        return NGX_CONF_ERROR;
> +    }
> +
> +    ngx_memzero(&ccv, sizeof(ngx_http_compile_complex_value_t));
> +
> +    ccv.cf = cf;
> +    ccv.value = &value[1];
> +    ccv.complex_value = param;
> +    ccv.zero = 1;
> +
> +    if (ngx_http_compile_complex_value(&ccv) != NGX_OK) {
> +        return NGX_CONF_ERROR;
> +    }
> +
> +    return NGX_CONF_OK;
> +}
> +
> +
> +

2 blank lines between functions, please.  3 blank lines before 
ngx_http_xslt_stylesheet() is a bug, not a rule.

Another question to consider: there was at least one request to 
simplify passing XPath expressions in params by allowing ':', see 
here:

http://mailman.nginx.org/pipermail/nginx-devel/2010-July/000414.html
http://mailman.nginx.org/pipermail/nginx-devel/2010-September/000486.html

With separate directive we may want to allow this.  Not sure 
though, probably it would be better to preserve compatibility with 
a way how parameters in xslt_stylesheet directive are handled.

>  static void
>  ngx_http_xslt_cleanup_dtd(void *data)
>  {
> @@ -931,6 +983,37 @@
>  
>  
>  static char *
> +ngx_http_xslt_filter_merge_params(const ngx_array_t *src, ngx_array_t *dest)
> +{
> +    ngx_uint_t i;
> +
> +    /* copy params */
> +    if (src->nelts && !dest->nelts) {
> +        dest->elts = src->elts;
> +        dest->nelts = src->nelts;
> +    }
> +    /* join params */
> +    else if (src->nelts && dest->nelts) {
> +        ngx_http_complex_value_t *params;
> +
> +        params = src->elts;
> +        for (i = 0; i < src->nelts; i++) {
> +            ngx_http_complex_value_t *new;
> +
> +            new = ngx_array_push(dest);
> +            if (new == NULL) {
> +                return NGX_CONF_ERROR;
> +            }
> +
> +    	    *new = params[i]; /* copy */
> +        }
> +    }
> +
> +    return NGX_CONF_OK;
> +}
> +
> +
> +static char *
>  ngx_http_xslt_filter_merge_conf(ngx_conf_t *cf, void *parent, void *child)
>  {
>      ngx_http_xslt_filter_loc_conf_t *prev = parent;
> @@ -940,9 +1023,26 @@
>          conf->dtd = prev->dtd;
>      }
>  
> +    if (ngx_http_xslt_filter_merge_params(&prev->params, &conf->params)
> +        != NGX_CONF_OK) {
> +        return NGX_CONF_ERROR;
> +    }

As I already said in the previous review, params should follow the 
same logic as other array-type directives (access_log, 
proxy_set_header, fastcgi_param, error_page, ...).  I.e. if you 
define any directive on a particular level, previous level data 
should not be inherited:

    location / {
        xslt_param foo=1;

        location /bar/ {
            xslt_param bar=1;
        }
    }

In location /bar/ only "bar=1" parameter should be present.

Please also note that with your code it's not possible to clear 
inherited params at all.

> +
>      if (conf->sheets.nelts == 0) {
>          conf->sheets = prev->sheets;
>      }
> +    else {
> +        ngx_http_xslt_sheet_t *sheet;
> +        ngx_uint_t             i;
> +
> +        sheet = conf->sheets.elts;
> +        for (i = 0; i < conf->sheets.nelts; i++) {
> +            if (ngx_http_xslt_filter_merge_params(&conf->params, &sheet[i].params)
> +                 != NGX_CONF_OK) {
> +               return NGX_CONF_ERROR;
> +            }
> +        }
> +    }

This is wrong.  Consider configuration like this:

    location / {
        xslt_stylesheet root.xslt;

        location /foo/ {
            xslt_params foo=bar;
        }
    }

With the above merge logic in location /foo/ the root.xslt 
stylesheet will be used without parameters, while "foo=bar" is 
clearly expected.

I suggest to use simple array-type inheritance for params, like

    if (conf->params == NULL) {
        conf->params = prev->params;
    }

and apply them separately to the stylesheet at runtime.  This will 
resolve both problems outlined above.

>  
>      if (ngx_http_merge_types(cf, &conf->types_keys, &conf->types,
>                               &prev->types_keys, &prev->types,
> @@ -957,7 +1057,7 @@
>  
>  
>  static ngx_int_t
> -ngx_http_xslt_filter_init(ngx_conf_t *cf)
> +ngx_http_xslt_filter_preinit(ngx_conf_t *cf)
>  {
>      xmlInitParser();
>  
> @@ -965,6 +1065,13 @@
>      exsltRegisterAll();
>  #endif
>  
> +    return NGX_OK;
> +}
> +
> +
> +static ngx_int_t
> +ngx_http_xslt_filter_init(ngx_conf_t *cf)
> +{
>      ngx_http_next_header_filter = ngx_http_top_header_filter;
>      ngx_http_top_header_filter = ngx_http_xslt_header_filter;
>  

See above, this is unrelated change.

Maxim Dounin



More information about the nginx-devel mailing list