anomalizer / ngx_aws_auth

nginx module to proxy to authenticated AWS services
BSD 2-Clause "Simplified" License
470 stars 144 forks source link

Only return NGX_OK if module is enabled #42

Closed DBezemer closed 7 years ago

DBezemer commented 7 years ago

Resolution for #41

DBezemer commented 7 years ago

@anomalizer while possible I believe checking for the module to be enabled and if not exiting with NGX_DECLINED is the right way to deal with modules that are active in the access control chain. The only reason the changeset affects more lines is that I removes the if {} clause around multiple lines.

In pseudocode:

if (!not) { 
    return; 
}
do stuff;
return;

Is more efficient (1 lower npath complexity) than:

if (is) {
    do stuff;
    return;
} else {
    return;
}
jessp01 commented 7 years ago

@anomalizer, I agree with @DBezemer. Checking:

if(!conf->enabled) 

and returning with NGX_DECLINED is more efficient and IMHO, also easier to read. Not sure why you feel adding else makes for a smaller change set, can you please explain?

Also, the patchset looks a lot bigger than it actually is because of different indentation styles, really the only addition is:

+    if(!conf->enabled) {
+        /* return directly if module is not enabled */
+        return NGX_DECLINED;
+    }

and of course, the removal of if(conf->enabled) { since with the code change, we'd never get to:

ngx_table_elt_t  *h;
header_pair_t *hv;

if the conf is not enabled.

jessp01 commented 7 years ago

@anomalizer, thanks for merging. Can you please issue a tag [release]? Thanks in advance.

anomalizer commented 7 years ago

@jessp01 tag 2.1.1 is released