Skip to content
This repository has been archived by the owner on Feb 24, 2024. It is now read-only.

Fix slicing of resource attributes during generation #1542

Merged
merged 7 commits into from
Jan 25, 2019
Merged

Fix slicing of resource attributes during generation #1542

merged 7 commits into from
Jan 25, 2019

Conversation

tobiashienzsch
Copy link
Contributor

See Issue #1540

@tobiashienzsch tobiashienzsch requested a review from a team as a code owner January 17, 2019 14:02
lukasschlueter
lukasschlueter previously approved these changes Jan 17, 2019
@tobiashienzsch
Copy link
Contributor Author

Seems like the integration tests failed on travis. I did not run those locally. :(

@lukasschlueter
Copy link
Contributor

Error message is Error: runtime error: slice bounds out of range.

I'm only on mobile until this evening, will have a look at why this test might be failing.

Maybe you can have a look at it as well?
The command for running integration tests is go test -v -tags "sqlite integration_test" -race ./...

https://github.com/gobuffalo/buffalo/blob/development/.travis.yml#L37-L38

@markbates
Copy link
Member

There needs to be an if test around that bit to make sure there are more than 1 thing in the args. That test is failing because only the name of the resource is being passed in without any fields. Add that if check and it should be good to go.

@markbates markbates added this to the v0.14.0 milestone Jan 18, 2019
@tobiashienzsch
Copy link
Contributor Author

I think I fixed it.

// Check if any attributes are given
if len(args) > 1 {
   ats, err := attrs.ParseArgs(args[1:]...)
   if err != nil {
	   return errors.WithStack(err)
   }
  resourceOptions.Attrs = ats
}

But I'm getting confused with all the auto generated files, that are being created during make test & make install. For example the file github.com/gobuffalo/buffalo/packrd/packed-packr.go The diff shows that every single hashed changed, is that normal? Should I run packr clean before commiting?

@tobiashienzsch
Copy link
Contributor Author

And we have another problem. With my fix, the generated templates have the correct field, but the model & migrations are missing the first one:

buffalo generate resource restaurant name address web

Output:
Model

type Restaurant struct {
	ID        uuid.UUID `json:"id" db:"id"`
	CreatedAt time.Time `json:"created_at" db:"created_at"`
	UpdatedAt time.Time `json:"updated_at" db:"updated_at"`
	Address   string    `json:"address" db:"address"`
	Web       string    `json:"web" db:"web"`
}

Template

<%= f.InputTag("Name") %>
<%= f.InputTag("Address") %>
<%= f.InputTag("Web") %>
<button class="btn btn-success" role="submit">Save</button>

It seems like the error (index mismatch) is happening later in the chain

@markbates
Copy link
Member

https://github.com/gobuffalo/buffalo/blob/development/genny/resource/models.go#L12 that's where the call to buffalo-pop is being made to generate the resources. That's probably not sending the correct payload.

@tobiashienzsch
Copy link
Contributor Author

I fixed it with the following changes:

func modelCommand(model name.Ident, opts *Options) *exec.Cmd {
	args := opts.Attrs.Slice()
	args = append(args[:0], args[0:]...) // Used to be: append(args[:0], args[0+1:]...)
        // ...

I'm a little confused by the second line since it doesn't seem to change anything. When I run:

func modelCommand(model name.Ident, opts *Options) *exec.Cmd {
	args := opts.Attrs.Slice()
	fmt.Println(args)
	fmt.Println(reflect.TypeOf(args))

	args = append(args[:0], args[0:]...)
	fmt.Println(args)
	fmt.Println(reflect.TypeOf(args))

The output is:

[category price]
[]string
[category price]
[]string

I can push my changes, but it would be nice to know how to handle the auto-generated files in that I talked about above. I don't want to commit unnecessary stuff.

@markbates
Copy link
Member

I've opened a PR with, what should be, the "correct" fixes. :)

#1547

@tobiashienzsch can you review/test and let me know if that works for you?

@tobiashienzsch
Copy link
Contributor Author

Looks good. No problems yet

@markbates
Copy link
Member

@tobiashienzsch do you want copy the changes from my PR to yours so you can get the, appropriate credit, for the work? You did a lot, I don't want to take that from you. I'm happy to kill my PR if you make the same changes in yours.

@tobiashienzsch
Copy link
Contributor Author

@markbates No not at all, that was by mistake. Not yet used to the GitHub workflow, I workly manly on my own. Sorry for the confusion.

@markbates
Copy link
Member

@tobiashienzsch you haven't done anything wrong at all. :) I opened my branch to help you. I want you to get full credit for fixing the problem, that's why I suggested you copy the changes into your branch, so we can merge your PR, and not mine. I don't want to take anything away from all the hard work you put into this.

@tobiashienzsch
Copy link
Contributor Author

Ok, that means, that I merge your branch into mine and then push again, right?

@tobiashienzsch
Copy link
Contributor Author

Travis failed with the same as on your commits, something with the sqlite test on windows.

@markbates
Copy link
Member

markbates commented Jan 24, 2019 via email

@lukasschlueter
Copy link
Contributor

lukasschlueter commented Jan 24, 2019

For me, the test for buffalo/cmd/internal/integration completed after 372 seconds, I'm not sure what the travis timeout is, maybe it took even longer on the travis machine?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants