error handling w/o error slices and more #157

jxsl13 · 2023-08-20T02:50:43Z

poc implementation for #155 & #117

benchmarks were broken of which only one is left broken.

Some findings that I had during my implementation is that there seem to be some Setter methods defined for value receiver methods (and some more staticcheck warnings)

And you need to dig pretty deep in order to find your configuration defaults. (http.Get for example)

Overall great test coverage which helped a lot.

Edit, just saw the other PR:
#123 seems to wrap errors already.
This PR does slightly more.
It allows to control circular reference errors at document object contruction for the top level library.

TODO:
~~use a tool like https://pkg.go.dev/golang.org/x/exp/cmd/gorelease to see how much the api changed.~~

…lar reference)

daveshanley · 2023-08-20T11:39:53Z

This will take some time to review! :)

jxsl13 · 2023-08-20T12:03:16Z

Another concern that I am not yet tackling neatly is how a user can easily work with those returned errors because errors.As or errors.Is would detect there to be a cyclic reference error inside of that slice but you would also need a (preferrably easy) way to check if there are also other errors inside of that multi error.

EDIT:
Maybe that MultiError (currently private type) must contain two levels of fields, one for hard errors and one for warnings

makes the user handle errors differently the user, depending in their requirement, will either want or not want to resolve circular references. In case that the user decides to forbid resolving circular references, they will handle returned errors as such and they will not need to pick out any specific error from the returned multi error. Changing this default removes the need to actually check if one of the errors is the circular reference error.

jxsl13 · 2023-08-20T17:43:02Z

Add dependabot
Update CI pipeline
fix many linter warnings

Allowed circular reference resolving by default

This makes the user handle errors differently.
The user, depending in their requirement, will either want or not want to resolve circular references.
In case that the user decides to forbid resolving circular references, they will handle returned errors as such and they will not need to pick out any specific error from the returned multi error.

Changing this default removes the need to actually check if one of the errors is the circular reference error.

in case a user wants to unwrap the MultiError, they should use a type assertion like this, I prefer not to bloat the public api with helper functions for error handling

doc, err := LoadDoc()
if err != nil {
    if errs, ok := err.(interface{ Unwrap() []error }); ok {
        for _, e := range errs.Unwrap() {
            fmt.Println(e)
        }
    }
}

github-advanced-security · 2023-08-21T17:42:49Z

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

index/find_component.go

jxsl13 · 2023-08-21T19:14:52Z

dunno what problems github currently has.
On a different project of mine I did not have any such problems at all.
Should rerun the action and ignore the security error, as the variable comes from filepath.Join

Tests will fail because of data races but not due to actual tests failing.

[]error -> error
update and harden ci/cd pipeline
fix some linter warnings (staticcheck)
Allow circular reference resolution by default
consolidate some errors (global error variables)
improve construction of new Documents, allow library user to specify stricter api spec validation (circular references)

same PR for my forked main branch:
test results: https://github.com/jxsl13/libopenapi/actions/runs/5919537497/job/16049980816?pr=1

daveshanley · 2023-08-25T16:40:15Z

The tests just timeout?

jxsl13 · 2023-08-25T22:13:53Z

the errors looked like something weird from the GitHub runner but not like the actual Go test timeout of 20 minutes was reached (which I configured).

jxsl13 · 2023-08-25T22:17:24Z

might be that the runner or assignment to a GitHub runner timed out

jxsl13 · 2023-08-26T09:43:59Z

first research indicates that high cpu load for an extended period of time might have caused the GitHub runner to have been canceled (crypto miner detection or whatever).

jxsl13 · 2023-08-26T11:21:31Z

seemingly splitting the tests and removing timeouts helps(?)

daveshanley · 2023-08-30T10:34:26Z

You should remove the -race flag. It totally crashes the runner, I am assuming these runners are provisioned with a single core and can't run race tests. Also all the race conditions have been cleaned up in #162

jxsl13 · 2023-08-30T14:34:55Z

will merge main branch into this branch once #162 is merged into main.

jxsl13 · 2023-09-01T19:14:55Z

slightly confused as to why this happened:

=== RUN   TestResolver_ResolveComponents_MixedRef
    resolver_test.go:282: 
        	Error Trace:	/home/runner/work/libopenapi/libopenapi/resolver/resolver_test.go:282
        	Error:      	Not equal: 
        	            	expected: 62
        	            	actual  : 63
        	Test:       	TestResolver_ResolveComponents_MixedRef
--- FAIL: TestResolver_ResolveComponents_MixedRef (0.01s)

running this locally returned 62 again.

codecov · 2023-09-02T11:19:31Z

Codecov Report

Patch coverage: 99.05% and project coverage change: -0.14% ⚠️

Comparison is base (1795bb5) 99.80% compared to head (c9cb487) 99.66%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #157      +/-   ##
==========================================
- Coverage   99.80%   99.66%   -0.14%     
==========================================
  Files         148      152       +4     
  Lines       10619    17810    +7191     
==========================================
+ Hits        10598    17750    +7152     
- Misses         21       39      +18     
- Partials        0       21      +21

Flag	Coverage Δ
unittests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
datamodel/low/v2/path_item.go	`100.00% <ø> (ø)`
datamodel/low/v3/path_item.go	`96.86% <ø> (-1.78%)`	⬇️
what-changed/model/components.go	`100.00% <ø> (ø)`
what-changed/model/path_item.go	`100.00% <ø> (ø)`
document.go	`96.44% <96.73%> (-1.70%)`	⬇️
index/find_component.go	`99.18% <99.17%> (-0.38%)`	⬇️
datamodel/high/base/schema.go	`100.00% <100.00%> (ø)`
datamodel/high/node_builder.go	`97.78% <100.00%> (-1.00%)`	⬇️
datamodel/high/v3/components.go	`100.00% <100.00%> (ø)`
datamodel/low/reference.go	`97.14% <100.00%> (-0.71%)`	⬇️
... and 10 more

... and 132 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jxsl13 · 2023-10-11T18:54:55Z

should be used as reference, errorutils might be replaced with simply errors.Join

jxsl13 added 8 commits August 20, 2023 01:36

initial error handling refactoring

c6f0308

fix some tests

1cc2a40

use filter func that is initialized only once

bd65eb9

return broken object upon error as expected in the tests

82f80e3

fix example

a3eb526

update lower level tests

8bc8538

do not return objects with errors

23ca2ac

fix tests, revert to returning objects even tho we have errors (circu…

f64b868

…lar reference)

jxsl13 changed the title ~~Poc/error handling~~ poc/error handling w/o error slices Aug 20, 2023

jxsl13 changed the title ~~poc/error handling w/o error slices~~ poc: error handling w/o error slices Aug 20, 2023

jxsl13 changed the title ~~poc: error handling w/o error slices~~ poc: error handling w/o error slices and more Aug 20, 2023

jxsl13 added 16 commits August 20, 2023 14:49

update ci go version to match go.mod version

18902b9

harden ci pipeline

b11b31a

add dependabot

beed338

go fmt ./...

6b2595b

fix some linter warnings

1a740b7

remove redundant break statements

1d14787

compile regex only once

f0d8af0

use copy intead of for loop

2af5be8

fix some linter warnings

3e55a41

fix some linter warnings

987651d

compile regex only once

06876cb

trim suffix with stdlib

5438afe

update test

127b681

update test to accomodate the default value change

745af79

fix example

38d5c31

make zero value config struct the same as the default config

9da55d5

github-advanced-security bot found potential problems Aug 21, 2023

View reviewed changes

index/find_component.go Dismissed Show resolved Hide resolved

jxsl13 changed the title ~~poc: error handling w/o error slices and more~~ error handling w/o error slices and more Aug 21, 2023

jxsl13 added 5 commits August 26, 2023 11:58

test splitting test, race & coverage

c3aa80c

Merge branch 'main' into poc/error-handling

e73fc93

update go.sum

b434c89

try w/o timeout

8c2c821

add makefile with the complete test commands

f6735fe

add t.Parallel to tests that don't have global shared stats

63207d2

rm -race flag

9ec2200

jxsl13 added 6 commits September 1, 2023 18:42

Merge branch 'main' into poc/error-handling

af5bb0d

fix build.yaml

6381834

fix multiple calls to t.Parallel()

04407cc

and filter logic -> or filter logic

e834c01

improve configuration & add tests

d2a8bc8

improve filter & make tests more understandable

b842c44

call all option functions for more test coverage

c9cb487

jxsl13 closed this Nov 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

error handling w/o error slices and more #157

error handling w/o error slices and more #157

jxsl13 commented Aug 20, 2023 •

edited

Loading

daveshanley commented Aug 20, 2023

jxsl13 commented Aug 20, 2023 •

edited

Loading

jxsl13 commented Aug 20, 2023 •

edited

Loading

github-advanced-security bot commented Aug 21, 2023

jxsl13 commented Aug 21, 2023 •

edited

Loading

daveshanley commented Aug 25, 2023

jxsl13 commented Aug 25, 2023

jxsl13 commented Aug 25, 2023

jxsl13 commented Aug 26, 2023

jxsl13 commented Aug 26, 2023 •

edited

Loading

daveshanley commented Aug 30, 2023

jxsl13 commented Aug 30, 2023

jxsl13 commented Sep 1, 2023 •

edited

Loading

codecov bot commented Sep 2, 2023

jxsl13 commented Oct 11, 2023 •

edited

Loading

error handling w/o error slices and more #157

error handling w/o error slices and more #157

Conversation

jxsl13 commented Aug 20, 2023 • edited Loading

daveshanley commented Aug 20, 2023

jxsl13 commented Aug 20, 2023 • edited Loading

jxsl13 commented Aug 20, 2023 • edited Loading

github-advanced-security bot commented Aug 21, 2023

jxsl13 commented Aug 21, 2023 • edited Loading

daveshanley commented Aug 25, 2023

jxsl13 commented Aug 25, 2023

jxsl13 commented Aug 25, 2023

jxsl13 commented Aug 26, 2023

jxsl13 commented Aug 26, 2023 • edited Loading

daveshanley commented Aug 30, 2023

jxsl13 commented Aug 30, 2023

jxsl13 commented Sep 1, 2023 • edited Loading

codecov bot commented Sep 2, 2023

Codecov Report

jxsl13 commented Oct 11, 2023 • edited Loading

jxsl13 commented Aug 20, 2023 •

edited

Loading

jxsl13 commented Aug 20, 2023 •

edited

Loading

jxsl13 commented Aug 20, 2023 •

edited

Loading

jxsl13 commented Aug 21, 2023 •

edited

Loading

jxsl13 commented Aug 26, 2023 •

edited

Loading

jxsl13 commented Sep 1, 2023 •

edited

Loading

jxsl13 commented Oct 11, 2023 •

edited

Loading