Solve panic issues - Githubissues

alovak commented 10 months ago

Closes #269

With this PR we:

introduce Validate() method for MessageSpec and field.Spec - so if you don't want constructor to panic, you can validate your spec before.
in StringsByHex if sorted value is not a Hex, we compare them as regular strings (no panic)
ignore panic in fuzzer, I hope later we can address it

codecov-commenter commented 10 months ago

Codecov Report

Patch coverage: 68.29% and project coverage change: +0.20% :tada:

Comparison is base (8252a73) 73.50% compared to head (c54575a) 73.70%. Report is 7 commits behind head on master.

:exclamation: Current head c54575a differs from pull request most recent head 828c2f1. Consider uploading reports for the commit 828c2f1 to get more accurate results

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #275 +/- ## ========================================== + Coverage 73.50% 73.70% +0.20% ========================================== Files 43 43 Lines 2291 2282 -9 ========================================== - Hits 1684 1682 -2 + Misses 377 369 -8 - Partials 230 231 +1 ``` | [Files Changed](https://app.codecov.io/gh/moov-io/iso8583/pull/275?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io) | Coverage Δ | | |---|---|---| | [prefix/ascii.go](https://app.codecov.io/gh/moov-io/iso8583/pull/275?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-cHJlZml4L2FzY2lpLmdv) | `71.42% <ø> (ø)` | | | [prefix/bcd.go](https://app.codecov.io/gh/moov-io/iso8583/pull/275?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-cHJlZml4L2JjZC5nbw==) | `69.69% <ø> (ø)` | | | [prefix/binary.go](https://app.codecov.io/gh/moov-io/iso8583/pull/275?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-cHJlZml4L2JpbmFyeS5nbw==) | `64.00% <ø> (ø)` | | | [prefix/ebcdic.go](https://app.codecov.io/gh/moov-io/iso8583/pull/275?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-cHJlZml4L2ViY2RpYy5nbw==) | `69.69% <ø> (ø)` | | | [prefix/ebcdic1047.go](https://app.codecov.io/gh/moov-io/iso8583/pull/275?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-cHJlZml4L2ViY2RpYzEwNDcuZ28=) | `87.50% <ø> (ø)` | | | [prefix/hex.go](https://app.codecov.io/gh/moov-io/iso8583/pull/275?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-cHJlZml4L2hleC5nbw==) | `72.41% <ø> (ø)` | | | [sort/strings.go](https://app.codecov.io/gh/moov-io/iso8583/pull/275?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-c29ydC9zdHJpbmdzLmdv) | `55.55% <0.00%> (ø)` | | | [test/fuzz-reader/reader.go](https://app.codecov.io/gh/moov-io/iso8583/pull/275?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-dGVzdC9mdXp6LXJlYWRlci9yZWFkZXIuZ28=) | `55.55% <0.00%> (ø)` | | | [message\_spec.go](https://app.codecov.io/gh/moov-io/iso8583/pull/275?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-bWVzc2FnZV9zcGVjLmdv) | `70.00% <25.00%> (-30.00%)` | :arrow_down: | | [message.go](https://app.codecov.io/gh/moov-io/iso8583/pull/275?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-bWVzc2FnZS5nbw==) | `71.94% <33.33%> (+0.64%)` | :arrow_up: | | ... and [3 more](https://app.codecov.io/gh/moov-io/iso8583/pull/275?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io) | | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/moov-io/iso8583/pull/275/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

PumpkinSeed commented 10 months ago

What do you think about creating a NewMessage version with error return? If someone wants to handle it on that way. (Speaking about my own case mostly)

For exmaple:

func NewMessage(spec *MessageSpec) *Message {
    message, err := newMessage(spec)
    if err != nil {
        panic(err) //nolint // as specs moslty static, we panic on spec validation errors
    }
    return message
}

// NOTE: I don't know what would be a good name for that
// NOTE2: There should be an explanation about backward compatibility
func NewMessageWithError(spec *MessageSpec) (*Message, error) {
    return newMessage(spec)
}

func newMessage(spec *MessageSpec) (*Message, error) {
    // Validate the spec
    if err := spec.Validate(); err != nil {
        return nil, err
    }
    .
    .
    ...
}

Let me know what you think about it.

PumpkinSeed commented 10 months ago

Also, it closes #269, not #270. :)

alovak commented 10 months ago

When you define spec, it will panic if you have a composite field with invalid spec. So, panic happens before NewMessage.

Panicking in Go is generally reserved for situations where it's inappropriate or impossible for the program to continue. In our case, if the spec is not valid, we can't proceed. I believe using panic is acceptable.

PumpkinSeed commented 10 months ago

When you define spec, it will panic if you have a composite field with invalid spec. So, panic happens before NewMessage.

Panicking in Go is generally reserved for situations where it's inappropriate or impossible for the program to continue. In our case, if the spec is not valid, we can't proceed. I believe using panic is acceptable.

By default I agree on that usage of panic and I'm not trying to push my point of view, but I would like to share some argument to add an exported function with error return. (Note, I don't want to remove the original behavior, I'm just suggesting a minor extension)

First I would like to highlight the usage of the library. By following the original documentation located in the README, we call the message := NewMessage(spec) with a specification defined as JSON and marshalled into the struct representation. We do that every time when we build a new transaction request and also we do that when an incoming message arrives. We do Unpack and Pack.

In idiomatic Go library users didn't expect that the library itself breaks the codes execution. That supposed to be the job of the programs control flow and the writer of the program should be able to take control when the program break it's execution.
In our use-case we are talking about multiple unreliable TCP connection which handled by multiple concurrent workers and channels for in-memory queues. (Probably it's our own failure to write something in that way) In this case one service handle multiple targets of iso8583 compliant backends. So getting a panic in such an unreliable environment puts extra job on the application, however if the error getting returned we could have handle it easily.
I know that the bad spec is not something what we need to execute anymore, but we have multiple different specs (because multiple payment processors has their own and we have at least 5 different one). That would be nice if in case of one bad specification it wouldn't break the entire application and let the scheduler decide whether it propagate the changes to the cluster based on the error log rate.

Based on these arguments I hope you can reconsider the suggestion above.

adamdecaf commented 10 months ago

I want to understand what you're asking us to reconsider. Are you asking that methods like NewMessage and SetSpec return error instead of panic?

PumpkinSeed commented 10 months ago

@adamdecaf I had this comment on this pull request.

PumpkinSeed commented 10 months ago

@adamdecaf Sorry I just answered quickly. By following the previously linked comment, I would like you to reconsider that you add an exported function which implements the same as NewMessage, but it returns with error instead of panic. I'm not the best in naming so I mentioned NewMessageWithError, but obviously it's not the best. Based on the reasons and arguments from above, this makes the user of the library able to decide whether the program can handle a panic in case of any issue or they want to handle it themselves by using this new exported function.

The main issue with the NewMessage that it has a lots of unexported part. So I can't write a function which is not relying on panic.

Also I understand if you don't want to add such a change, but I thought I try it. :)

adamdecaf commented 9 months ago

I don't think supporting backwards compatibility of panics is something we want to support. Causing a panic is very expensive and dangerous to systems. Callers can still panic if they wish, but our libraries always try to avoid that.

PumpkinSeed commented 9 months ago

I don't think supporting backwards compatibility of panics is something we want to support. Causing a panic is very expensive and dangerous to systems. Callers can still panic if they wish, but our libraries always try to avoid that.

I don't want to pick a side in this question, I just offered a possible solution where the caller of the library can decide between the backward compatible way or using another function with error return.

Otherwise I wrote a wrapper solution to avoid panic for our use-case. So I'm also fine with the current implementation as well.

moov-io / iso8583

Solve panic issues #275

Codecov Report