Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix slice pointer dive tag #43

Closed
wants to merge 1 commit into from

Conversation

zaneli
Copy link

@zaneli zaneli commented May 26, 2023

When orig is pointer, not only !current.CanAddr() but also else modify orig, so call t.extractType(orig) both cases.
If a value is set from nil,kind is reassigned from ptr to slice, map.

And I would prefer to allow the following cases.

type Foo struct { Bars: *[]Bar `mold:"dive"` }

f1 := Foo{ Bars: &[]Bar{} } // No problem with the status quo.
f2 := Foo{ Bars: nil } // Now "invalid dive tag configuration" error returns.

@coveralls
Copy link

Coverage Status

Coverage: 96.439% (+0.02%) from 96.418% when pulling c606cb8 on zaneli:fix/dive-nil into 05050dc on go-playground:master.

@deankarn
Copy link
Contributor

Is this also true if the slice is not a pointer to a slice?

I ask because you should never, except in some wild edge cases, ever hav a pointer to a slice.

@zaneli
Copy link
Author

zaneli commented May 27, 2023

Is this also true if the slice is not a pointer to a slice?

No.

I generate some codes using oapi-codegen.
type: array and not required field, oapi-codegen generate a pointer to a slice.

components:
  schemas:
    FooResponse:
      type: object
      required:
      - id
      properties:
        id:
          type: string
        attributes:
          type: array
          items:
            $ref: "#/components/schemas/Attribute"
          x-oapi-codegen-extra-tags:
            mold: dive
type FooResponse struct {
	ID         string       `json:"id"`
	Attributes *[]Attribute `json:"attributes,omitempty"` `mold:"dive"`
}

I ask because you should never, except in some wild edge cases, ever hav a pointer to a slice.

OK, I will try to address this with some sort of work around.
(or consider quitting using the two libraries in combination.)

Thank you.

@zaneli zaneli closed this May 27, 2023
@zaneli zaneli deleted the fix/dive-nil branch May 27, 2023 13:21
@deankarn
Copy link
Contributor

deankarn commented May 27, 2023

OK do what you must @zaneli , but seems like a bit of an overreaction.

I didn’t say I wasn’t interested in fixing or handling this unusual case but was rather gathering more information to understand the problem in more detail.

As it turns out you’re not in control of the type being generated by oapi-codegen is the reason why it’s a pointer to a slice(which the codegen is incorrectly generating because slice is already a smart pointer and so already nil by default).

Now that I have this additional information I can evaluate your proposed fix and think about/ensure it’s not a breaking change or if theres even a better/different way to handle also.

Additionally there are existing workarounds in the meantime, such as using the 'default' modifier before the dive tag to create the empty array when nothing is there, which should work.. but haven’t tested yet.

@zaneli
Copy link
Author

zaneli commented May 27, 2023

Thanks for your kind response, it is much appreciated.

Additionally there are existing workarounds in the meantime, such as using the 'default' modifier before the dive tag to create the empty array when nothing is there, which should work.. but haven’t tested yet.

Unfortunately it doesn't seem to work fine now.
That is why I moved current, kind = t.extractType(orig) from line 260 to line 274 in this PR.

After creating an empty array, the kind is not updated, so it remains reflect.Ptr.
Either way, an ErrInvalidDive error returns when evaluating the dive tag afterwards.

@deankarn
Copy link
Contributor

Fwiw I will be trying to look at it this weekend barring any RL(real life) interrupts :)

@deankarn
Copy link
Contributor

@zaneli would you be able to share how you initialize the mold library?

Asking because after doing some preliminary testing to reproduce I was unable to using modifiers.New() but realized because you're using the mold tag and not mod that you may now be initializing it the same way I am.

@zaneli

This comment was marked as resolved.

@zaneli
Copy link
Author

zaneli commented May 27, 2023

I also realized that creating a default empty array does not fit my needs.
I would like to distinguish between these two, but setting a default always results in the latter.

{"id":"id1"}
{"id":"id1","attributes":[]}

@deankarn deankarn mentioned this pull request May 28, 2023
deankarn added a commit that referenced this pull request May 28, 2023
- Added new default & set types for `chan`, `map`, `slice`, `time.Time`
and `Pointer` types.
- Updated to handle pointer to a `Slice` or `Map` for situations where
code is not under your control. Fixes #43
@deankarn
Copy link
Contributor

@zaneli PR #44 should address both.

You should now be able to set a default, if that's what is desired and I've also added handling of nil pointer to Slice and Map that's close to the original PR you made.

The change differed to keep usage safety, the original PR would have allowed this to pass without error where the new changes won't let the invalid usage pass:

type Test struct {
    S *string `mod:"dive"`
}

Let me know if this fully resolves your use case :)

@zaneli
Copy link
Author

zaneli commented May 29, 2023

Thank you for the correction.
I have confirmed that the issue has been resolved.

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

Successfully merging this pull request may close these issues.

None yet

3 participants