development: strict types could be returned from handlers #1903

onokonem · 2019-03-17T11:48:04Z

to make code more safe

…e more safe Signed-off-by: Daniel Podolsky <onokonem@gmail.com>

generator/shared.go

Signed-off-by: Daniel Podolsky <onokonem@gmail.com>

codecov · 2019-03-17T12:14:22Z

Codecov Report

Merging #1903 into master will decrease coverage by 0.05%.
The diff coverage is 76.66%.

@@            Coverage Diff             @@
##           master    #1903      +/-   ##
==========================================
- Coverage   80.74%   80.68%   -0.06%     
==========================================
  Files          38       38              
  Lines        7583     7669      +86     
==========================================
+ Hits         6123     6188      +65     
- Misses        981      994      +13     
- Partials      479      487       +8

Impacted Files	Coverage Δ
generator/structs.go	`74.76% <ø> (ø)`	⬆️
generator/template_repo.go	`87.96% <ø> (ø)`	⬆️
generator/operation.go	`89.14% <100%> (+0.01%)`	⬆️
cmd/swagger/commands/generate/server.go	`100% <100%> (ø)`	⬆️
generator/shared.go	`82.62% <40%> (-1.03%)`	⬇️
generator/bindata.go	`72.54% <83.33%> (+1.62%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 873540c...b50d2a2. Read the comment docs.

codecov · 2019-03-17T12:14:22Z

Codecov Report

Merging #1903 into master will decrease coverage by 0.25%.
The diff coverage is 76.66%.

@@            Coverage Diff             @@
##           master    #1903      +/-   ##
==========================================
- Coverage   80.93%   80.68%   -0.26%     
==========================================
  Files          38       38              
  Lines        7586     7669      +83     
==========================================
+ Hits         6140     6188      +48     
- Misses        964      994      +30     
- Partials      482      487       +5

Impacted Files	Coverage Δ
generator/template_repo.go	`87.96% <ø> (ø)`	⬆️
generator/structs.go	`74.76% <ø> (+1.13%)`	⬆️
cmd/swagger/commands/generate/server.go	`100% <100%> (ø)`	⬆️
generator/operation.go	`89.14% <100%> (+0.01%)`	⬆️
generator/shared.go	`82.62% <40%> (-2.77%)`	⬇️
generator/bindata.go	`72.54% <83.33%> (+1.62%)`	⬆️
generator/types.go	`90.88% <0%> (-0.96%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3123666...1c05f2b. Read the comment docs.

casualjim · 2019-03-17T17:33:49Z

Do you have an example of what the end result looks like? Maybe in the examples folder?

onokonem · 2019-03-17T18:15:36Z

of course I do :)

configure_issue_tracker.go

this is a file generated from examples/task-tracker. there are some more changes but they all are related to the changes in this file.

The main idea is to force the handler to return one of the replays predefined for this particular method.

onokonem · 2019-03-17T18:16:40Z

and yes, I can create a strict (and build api) examples based on the existing ones. just tell me which one suits best

casualjim · 2019-03-17T18:23:23Z

Either the todo list api or the issue tracker example would be great

Signed-off-by: Daniel Podolsky <onokonem@gmail.com>

onokonem · 2019-03-17T19:56:59Z

done

onokonem · 2019-03-17T19:58:19Z

BTW, I need help adding tests for these options.

casualjim · 2019-03-17T20:00:48Z

so essentially the change is it returns the more precise responder types instead of the interface?
And the builder changes because of that, including the not implemented error?

onokonem · 2019-03-17T20:01:25Z

exactly

casualjim · 2019-03-17T20:05:45Z

for tests we typically take a spec, generate code and verify that the files have the expected content through regex matchers etc.

Signed-off-by: Daniel Podolsky <onokonem@gmail.com>

casualjim · 2019-03-19T06:36:46Z

I suppose this is not ready for merge yet is that correct?

onokonem · 2019-03-19T07:17:08Z

from my point of view it is ready to be merged

it is in use in prod in some place at the moment

the parts are missing:

tests to satisfy the coverage checker
tests to actual test the functionality

unfortunately it looks like I'll not complete the tests

casualjim · 2019-03-20T07:30:39Z

Unfortunately we need tests to accept this contribution, so that it doesn't inadvertently breaks. The last year we spent increasing our test coverage and stability.

Signed-off-by: Daniel Podolsky <onokonem@gmail.com>

onokonem · 2019-03-25T06:33:25Z

Unfortunately we need tests to accept this contribution, so that it doesn't inadvertently breaks. The last year we spent increasing our test coverage and stability.

do you have your test system documented somewhere?

I might be able to assign somebody less busy/lazy to this task

casualjim · 2019-03-25T06:38:21Z

I don't think we have it documented somewhere but it's pretty straight forward. You make a spec or take one of the pre-existing ones, and then call render. We have tons of tests that all look like this:

https://github.com/go-swagger/go-swagger/blob/master/generator/discriminators_test.go#L38-L65

Or more in line with this changeset:
https://github.com/go-swagger/go-swagger/blob/master/generator/server_test.go#L347-L379

fredbi · 2019-03-31T18:53:00Z

@onokonem I've had a look at this work and was surprised about the way you are duplicating a lot of templates.
Is this change so hard on response, etc templates so as to require an entire copy of this source?

Couldn't the same be achieved with proper {{ if .Stricts }} ... {{ else }} ... {{ end }} with a single template?

onokonem · 2019-03-31T20:23:58Z

Couldn't the same be achieved with proper {{ if .Stricts }} ... {{ else }} ... {{ end }} with a single template?

@fredbi , of course it could

But these templates are overcomplicated already.

fredbi · 2019-03-31T20:39:17Z

But these templates are overcomplicated already.

So you think that duplicating the source would make the overall solution simpler to maintain and safer to use? I don't think anyone is going to apply their changes everywhere and maintain both sets of templates.

Another way to "fork out" templates is to add "contrib" templates, which live their own lives next to the main templates set.

onokonem · 2019-03-31T20:50:36Z

Another way to "fork out" templates is to add "contrib" templates, which live their own lives next to the main templates set

I've tried that first, but found I need one more parameter passed to templates.

So I've added them

For my own task I've added the templates we are using to the project.

And yes, I'm ready to discuss how this has to be refactored to be added to the upstream, as it is a little pain to maintain the fork.

fredbi · 2019-04-01T19:02:45Z

generator/template_repo.go

@@ -150,6 +150,13 @@ var assets = map[string][]byte{
 	"client/response.gotmpl":  MustAsset("templates/client/response.gotmpl"),
 	"client/client.gotmpl":    MustAsset("templates/client/client.gotmpl"),
 	"client/facade.gotmpl":    MustAsset("templates/client/facade.gotmpl"),
+
+	"server/builder-strict.gotmpl":      MustAsset("templates/server/builder-strict.gotmpl"),
+	"server/configureapi-strict.gotmpl": MustAsset("templates/server/configureapi-strict.gotmpl"),


This duplication of templates should be avoided. Parameters passed to templates are settable through the structs.go definitions and definitions in shared.go

fredbi · 2019-04-01T19:06:55Z

generator/shared.go

@@ -432,15 +432,15 @@ func DefaultSectionOpts(gen *GenOpts) {
 			if gen.IncludeResponses {
 				ops = append(ops, TemplateOpts{
 					Name:     "responses",
-					Source:   "asset:serverResponses",
+					Source:   "asset:serverResponses" + stringTernary(gen.Strict, "Strict", ""),


Extra complexity that should be avoided.

fredbi · 2019-04-01T19:09:01Z

generator/shared.go

@@ -580,6 +588,9 @@ type GenOpts struct {
 	CompatibilityMode      string
 	ExistingModels         string
 	Copyright              string
+
+	Strict          bool


GenOpts are available from the template, so there is no need to duplicate templates

fredbi · 2019-04-01T19:10:43Z

generator/shared.go

@@ -1257,3 +1268,10 @@ func sharedValidationsFromSchema(v spec.Schema, isRequired bool) (sh sharedValid
 	}
 	return
 }
+
+func stringTernary(cond bool, ifTrue string, ifFalse string) string { // nolint: unparam


the // nolint: hint is in my opinion unnecessary. You could just declare this has: (cond bool, ifTrue, ifFalse string) string

onokonem · 2019-04-03T11:03:27Z

great, will do on weekend

onokonem · 2020-06-12T23:01:08Z

replaced by #2312

development: strict types could be returned from handlers to make cod…

b9a4968

…e more safe Signed-off-by: Daniel Podolsky <onokonem@gmail.com>

golangcibot reviewed Mar 17, 2019

View reviewed changes

generator/shared.go Outdated Show resolved Hide resolved

fix: useless linter warning disabled

b50d2a2

Signed-off-by: Daniel Podolsky <onokonem@gmail.com>

development: strict and buildapi examples

0fa197a

Signed-off-by: Daniel Podolsky <onokonem@gmail.com>

onokonem added 3 commits March 17, 2019 23:26

fix: improper paths in example generated code

b2cbbf4

Signed-off-by: Daniel Podolsky <onokonem@gmail.com>

improvement: more clean BuildAPI logic

4bc7534

Signed-off-by: Daniel Podolsky <onokonem@gmail.com>

fix: not tagged methods has to be generated properly

ed147b3

Signed-off-by: Daniel Podolsky <onokonem@gmail.com>

Merge branch 'master' into clean-more-safety

bc07263

fix: useless configureAPI() call removed from BuildAPI()

f704895

Signed-off-by: Daniel Podolsky <onokonem@gmail.com>

Merge branch 'master' into clean-more-safety

1c05f2b

fredbi requested changes Apr 1, 2019

View reviewed changes

onokonem closed this Jun 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

development: strict types could be returned from handlers #1903

development: strict types could be returned from handlers #1903

onokonem commented Mar 17, 2019

codecov bot commented Mar 17, 2019

codecov bot commented Mar 17, 2019 •

edited

Loading

casualjim commented Mar 17, 2019

onokonem commented Mar 17, 2019

onokonem commented Mar 17, 2019

casualjim commented Mar 17, 2019

onokonem commented Mar 17, 2019

onokonem commented Mar 17, 2019

casualjim commented Mar 17, 2019

onokonem commented Mar 17, 2019

casualjim commented Mar 17, 2019

casualjim commented Mar 19, 2019

onokonem commented Mar 19, 2019

casualjim commented Mar 20, 2019

onokonem commented Mar 25, 2019

casualjim commented Mar 25, 2019

fredbi commented Mar 31, 2019

onokonem commented Mar 31, 2019

fredbi commented Mar 31, 2019

onokonem commented Mar 31, 2019

fredbi Apr 1, 2019

fredbi Apr 1, 2019

fredbi Apr 1, 2019

fredbi Apr 1, 2019

onokonem commented Apr 3, 2019

onokonem commented Jun 12, 2020

development: strict types could be returned from handlers #1903

development: strict types could be returned from handlers #1903

Conversation

onokonem commented Mar 17, 2019

codecov bot commented Mar 17, 2019

Codecov Report

codecov bot commented Mar 17, 2019 • edited Loading

Codecov Report

casualjim commented Mar 17, 2019

onokonem commented Mar 17, 2019

onokonem commented Mar 17, 2019

casualjim commented Mar 17, 2019

onokonem commented Mar 17, 2019

onokonem commented Mar 17, 2019

casualjim commented Mar 17, 2019

onokonem commented Mar 17, 2019

casualjim commented Mar 17, 2019

casualjim commented Mar 19, 2019

onokonem commented Mar 19, 2019

casualjim commented Mar 20, 2019

onokonem commented Mar 25, 2019

casualjim commented Mar 25, 2019

fredbi commented Mar 31, 2019

onokonem commented Mar 31, 2019

fredbi commented Mar 31, 2019

onokonem commented Mar 31, 2019

fredbi Apr 1, 2019

Choose a reason for hiding this comment

fredbi Apr 1, 2019

Choose a reason for hiding this comment

fredbi Apr 1, 2019

Choose a reason for hiding this comment

fredbi Apr 1, 2019

Choose a reason for hiding this comment

onokonem commented Apr 3, 2019

onokonem commented Jun 12, 2020

codecov bot commented Mar 17, 2019 •

edited

Loading