mirror of https://github.com/stashapp/stash.git
11 Commits
| Author | SHA1 | Message | Date |
|---|---|---|---|
kermieisinthehouse
|
30879389ec
|
Fix golangci OOM (#2889)
* Fix golangci OOM * Fix all lints |
|
|
SmallCoccinelle
|
401660e6a3
|
Hoist context, enable errchkjson (#2488)
* Make the script scraper context-aware Connect the context to the command execution. This means command execution can be aborted if the context is canceled. The context is usually bound to user-interaction, i.e., a scraper operation issued by the user. Hence, it seems correct to abort a command if the user aborts. * Enable errchkjson Some json marshal calls are *safe* in that they can never fail. This is conditional on the types of the the data being encoded. errchkjson finds those calls which are unsafe, and also not checked for errors. Add logging warnings to the place where unsafe encodings might happen. This can help uncover usage bugs early in stash if they are tripped, making debugging easier. While here, keep the checker enabled in the linter to capture future uses of json marshalling. * Pass the context for zip file scanning. * Pass the context in scanning * Pass context, replace context.TODO() Where applicable, pass the context down toward the lower functions in the call stack. Replace uses of context.TODO() with the passed context. This makes the code more context-aware, and you can rely on aborting contexts to clean up subsystems to a far greater extent now. I've left the cases where there is a context in a struct. My gut feeling is that they have solutions that are nice, but they require more deep thinking to unveil how to handle it. * Remove context from task-structs As a rule, contexts are better passed explicitly to functions than they are passed implicitly via structs. In the case of tasks, we already have a valid context in scope when creating the struct, so remove ctx from the struct and use the scoped context instead. With this change it is clear that the scanning functions are under a context, and the task-starting caller has jurisdiction over the context and its lifetime. A reader of the code don't have to figure out where the context are coming from anymore. While here, connect context.TODO() to the newly scoped context in most of the scan code. * Remove context from autotag struct too * Make more context-passing explicit In all of these cases, there is an applicable context which is close in the call-tree. Hook up to this context. * Simplify context passing in manager The managers context handling generally wants to use an outer context if applicable. However, the code doesn't pass it explicitly, but stores it in a struct. Pull out the context from the struct and use it to explicitly pass it. At a later point in time, we probably want to handle this by handing over the job to a different (program-lifetime) context for background jobs, but this will do for a start. |
|
|
SmallCoccinelle
|
45f700d6ea
|
Support Go 1.18: Upgrade gqlgen to v0.17.2 (#2443)
* Upgrade gqlgen to v0.17.2 This enables builds on Go 1.18. github.com/vektah/gqlparser is upgraded to the newest version too. Getting this to work is a bit of a hazzle. I had to first remove vendoring from the repository, perform the upgrade and then re-introduce the vendor directory. I think gqlgens analysis went wrong for some reason on the upgrade. It would seem a clean-room installation fixed it. * Bump project to 1.18 * Update all packages, address gqlgenc breaking changes * Let `go mod tidy` handle the go.mod file * Upgrade linter to 1.45.2 * Introduce v1.45.2 of the linter The linter now correctly warns on `strings.Title` because it isn't unicode-aware. Fix this by using the suggested fix from x/text/cases to produce unicode-aware strings. The mapping isn't entirely 1-1 as this new approach has a larger iface: it spans all of unicode rather than just ASCII. It coincides for ASCII however, so things should be largely the same. * Ready ourselves for errchkjson and contextcheck. * Revert dockerfile golang version changes for now Co-authored-by: Kermie <kermie@isinthe.house> Co-authored-by: WithoutPants <53250216+WithoutPants@users.noreply.github.com> |
|
|
SmallCoccinelle
|
8b7720e3bf
|
Enable the appendAssign lint check (#1865)
* Make copies of buffers Avoid reusing one of the incoming arrays as a append extension, and make a copy of the data. It's cleaner in the long run and possibly easier for the GC to maintain. * Avoid appendAssign problems in tag code Reuse the existing slice when appending. * Fix appendAssign in encoder_scene_preview_chunk Appending and creating a new slice is somewhat unintuitive since the underlying slice might be extended to satisfy the new capacity. This sometimes leads to faulty logic. Rewrite the code so it reuses `args` for all appending, and builds one array clearly in the code. It follows the general style of the function where `args` is being built in small incremental batches and avoids the introduction of new names. * Enable the appendAssign check This makes us pass all gocritic warnings. Co-authored-by: WithoutPants <53250216+WithoutPants@users.noreply.github.com> |
|
|
SmallCoccinelle
|
214a15bc40
|
Fixups + enable the commentFormatting linter (#1866)
* Add a space after // comments
For consistency, the commentFormatting lint checker suggests a space
after each // comment block. This commit handles all the spots in
the code where that is needed.
* Rewrite documentation on functions
Use the Go idiom of commenting:
* First sentence declares the purpose.
* First word is the name being declared
The reason this style is preferred is such that grep is able to find
names the user might be interested in. Consider e.g.,
go doc -all pkg/ffmpeg | grep -i transcode
in which case a match will tell you the name of the function you are
interested in.
* Remove old code comment-blocks
There are some commented out old code blocks in the code base. These are
either 3 years old, or 2 years old. By now, I don't think their use is
going to come back any time soon, and Git will track old pieces of
deleted code anyway.
Opt for deletion.
* Reorder imports
Split stdlib imports from non-stdlib imports in files we are touching.
* Use a range over an iteration variable
Probably more go-idiomatic, and the code needed comment-fixing anyway.
* Use time.After rather than rolling our own
The idiom here is common enough that the stdlib contains a function for
it. Use the stdlib function over our own variant.
* Enable the commentFormatting linter
|
|
|
SmallCoccinelle
|
e14bb8432c
|
Enable gocritic (#1848)
* Don't capitalize local variables
ValidCodecs -> validCodecs
* Capitalize deprecation markers
A deprecated marker should be capitalized.
* Use re.MustCompile for static regexes
If the regex fails to compile, it's a programmer error, and should be
treated as such. The regex is entirely static.
* Simplify else-if constructions
Rewrite
else { if cond {}}
to
else if cond {}
* Use a switch statement to analyze formats
Break an if-else chain. While here, simplify code flow.
Also introduce a proper static error for unsupported image formats,
paving the way for being able to check against the error.
* Rewrite ifElse chains into switch statements
The "Effective Go" https://golang.org/doc/effective_go#switch document
mentions it is more idiomatic to write if-else chains as switches when
it is possible.
Find all the plain rewrite occurrences in the code base and rewrite.
In some cases, the if-else chains are replaced by a switch scrutinizer.
That is, the code sequence
if x == 1 {
..
} else if x == 2 {
..
} else if x == 3 {
...
}
can be rewritten into
switch x {
case 1:
..
case 2:
..
case 3:
..
}
which is clearer for the compiler: it can decide if the switch is
better served by a jump-table then a branch-chain.
* Rewrite switches, introduce static errors
Introduce two new static errors:
* `ErrNotImplmented`
* `ErrNotSupported`
And use these rather than forming new generative errors whenever the
code is called. Code can now test on the errors (since they are static
and the pointers to them wont change).
Also rewrite ifElse chains into switches in this part of the code base.
* Introduce a StashBoxError in configuration
Since all stashbox errors are the same, treat them as such in the code
base. While here, rewrite an ifElse chain.
In the future, it might be beneifical to refactor configuration errors
into one error which can handle missing fields, which context the error
occurs in and so on. But for now, try to get an overview of the error
categories by hoisting them into static errors.
* Get rid of an else-block in transaction handling
If we succesfully `recover()`, we then always `panic()`. This means the
rest of the code is not reachable, so we can avoid having an else-block
here.
It also solves an ifElse-chain style check in the code base.
* Use strings.ReplaceAll
Rewrite
strings.Replace(s, o, n, -1)
into
strings.ReplaceAll(s, o, n)
To make it consistent and clear that we are doing an all-replace in the
string rather than replacing parts of it. It's more of a nitpick since
there are no implementation differences: the stdlib implementation is
just to supply -1.
* Rewrite via gocritic's assignOp
Statements of the form
x = x + e
is rewritten into
x += e
where applicable.
* Formatting
* Review comments handled
Stash-box is a proper noun.
Rewrite a switch into an if-chain which returns on the first error
encountered.
* Use context.TODO() over context.Background()
Patch in the same vein as everything else: use the TODO() marker so we
can search for it later and link it into the context tree/tentacle once
it reaches down to this level in the code base.
* Tell the linter to ignore a section in manager_tasks.go
The section is less readable, so mark it with a nolint for now. Because
the rewrite enables a ifElseChain, also mark that as nolint for now.
* Use strings.ReplaceAll over strings.Replace
* Apply an ifElse rewrite
else { if .. { .. } } rewrite into else if { .. }
* Use switch-statements over ifElseChains
Rewrite chains of if-else into switch statements. Where applicable,
add an early nil-guard to simplify case analysis. Also, in
ScanTask's Start(..), invert the logic to outdent the whole block, and
help the reader: if it's not a scene, the function flow is now far more
local to the top of the function, and it's clear that the rest of the
function has to do with scene management.
* Enable gocritic on the code base.
Disable appendAssign for now since we aren't passing that check yet.
* Document the nolint additions
* Document StashBoxBatchPerformerTagInput
|
|
|
SmallCoccinelle
|
655d3ae969
|
Toward better context handling (#1835)
* Use the request context
The code uses context.Background() in a flow where there is a
http.Request. Use the requests context instead.
* Use a true context in the plugin example
Let AddTag/RemoveTag take a context and use that context throughout
the example.
* Avoid the use of context.Background
Prefer context.TODO over context.Background deep in the call chain.
This marks the site as something which we need to context-handle
later, and also makes it clear to the reader that the context is
sort-of temporary in the code base.
While here, be consistent in handling the `act` variable in each
branch of the if .. { .. } .. check.
* Prefer context.TODO over context.Background
For the different scraping operations here, there is a context
higher up the call chain, which we ought to use. Mark the call-sites
as TODO for now, so we can come back later on a sweep of which parts
can be context-lifted.
* Thread context upwards
Initialization requires context for transactions. Thread the context
upward the call chain.
At the intialization call, add a context.TODO since we can't break this
yet. The singleton assumption prevents us from pulling it up into main for
now.
* make tasks context-aware
Change the task interface to understand contexts.
Pass the context down in some of the branches where it is needed.
* Make QueryStashBoxScene context-aware
This call naturally sits inside the request-context. Use it.
* Introduce a context in the JS plugin code
This allows us to use a context for HTTP calls inside the system.
Mark the context with a TODO at top level for now.
* Nitpick error formatting
Use %v rather than %s for error interfaces.
Do not begin an error strong with a capital letter.
* Avoid the use of http.Get in FFMPEG download chain
Since http.Get has no context, it isn't possible to break out or have
policy induced. The call will block until the GET completes. Rewrite
to use a http Request and provide a context.
Thread the context through the call chain for now. provide
context.TODO() at the top level of the initialization chain.
* Make getRemoteCDPWSAddress aware of contexts
Eliminate a call to http.Get and replace it with a context-aware
variant.
Push the context upwards in the call chain, but plug it before the
scraper interface so we don't have to rewrite said interface yet.
Plugged with context.TODO()
* Scraper: make the getImage function context-aware
Use a context, and pass it upwards. Plug it with context.TODO()
up the chain before the rewrite gets too much out of hand for now.
Minor tweaks along the way, remove a call to context.Background()
deep in the call chain.
* Make NOTIFY request context-aware
The call sits inside a Request-handler. So it's natural to use the
requests context as the context for the outgoing HTTP request.
* Use a context in the url scraper code
We are sitting in code which has a context, so utilize it for the
request as well.
* Use a context when checking versions
When we check the version of stash on Github, use a context. Thread
the context up to the initialization routine of the HTTP/GraphQL
server and plug it with a context.TODO() for now.
This paves the way for providing a context to the HTTP server code in a
future patch.
* Make utils func ReadImage context-aware
In almost all of the cases, there is a context in the call chain which
is a natural use. This is true for all the GraphQL mutations.
The exception is in task_stash_box_tag, so plug that task with
context.TODO() for now.
* Make stash-box get context-aware
Thread a context through the call chain until we hit the Client API.
Plug it with context.TODO() there for now.
* Enable the noctx linter
The code is now free of any uncontexted HTTP request. This means we
pass the noctx linter, and we can enable it in the code base.
|
|
|
SmallCoccinelle
|
c6f6205e4f
|
Errorlint sweep + minor linter tweaks (#1796)
* Replace error assertions with Go 1.13 style
Use `errors.As(..)` over type assertions. This enables better use of
wrapped errors in the future, and lets us pass some errorlint checks
in the process.
The rewrite is entirely mechanical, and uses a standard idiom for
doing so.
* Use Go 1.13's errors.Is(..)
Rather than directly checking for error equality, use errors.Is(..).
This protects against error wrapping issues in the future.
Even though something like sql.ErrNoRows doesn't need the wrapping, do
so anyway, for the sake of consistency throughout the code base.
The change almost lets us pass the `errorlint` Go checker except for
a missing case in `js.go` which is to be handled separately; it isn't
mechanical, like these changes are.
* Remove goconst
goconst isn't a useful linter in many cases, because it's false positive
rate is high. It's 100% for the current code base.
* Avoid direct comparison of errors in recover()
Assert that we are catching an error from recover(). If we are,
check that the error caught matches errStop.
* Enable the "errorlint" checker
Configure the checker to avoid checking for errorf wraps. These are
often false positives since the suggestion is to blanket wrap errors
with %w, and that exposes the underlying API which you might not want
to do.
The other warnings are good however, and with the current patch stack,
the code base passes all these checks as well.
* Configure rowserrcheck
The project uses sqlx. Configure rowserrcheck to include said package.
* Mechanically rewrite a large set of errors
Mechanically search for errors that look like
fmt.Errorf("...%s", err.Error())
and rewrite those into
fmt.Errorf("...%v", err)
The `fmt` package is error-aware and knows how to call err.Error()
itself.
The rationale is that this is more idiomatic Go; it paves the
way for using error wrapping later with %w in some sites.
This patch only addresses the entirely mechanical rewriting caught by
a project-side search/replace. There are more individual sites not
addressed by this patch.
|
|
|
SmallCoccinelle
|
a5ca8fc678
|
Enable safe linters (#1786)
* Enable safe linters Enable the linters dogsled, rowserrcheck, and sqlclosecheck. These report no errors currently in the code base. Enable misspell. Misspell finds two spelling mistakes in comments, which are fixed by the patch as well. Add and sort linters which are relatively safe to add over time. Comment them out for now. * Close the response body If we can get a HTTP response, it has a body which ought to be closed. By doing so, we avoid potentially leaking connections. * Enable the exportloopref linter There are two places in the code with these warnings. Fix them while enabling the linter. * Remove redundant types in tests If a slice already determines the type, the inner type declaration is redundant. Remove the inner declarations. * Mark autotag test cases as parallel Autotag test cases is by far the outlier when it comes to test time. While go test runs test cases in parallel, it doesn't do so inside a given package, unless one marks the test cases as parallel. This change provides a significant speedup on a 8-core machine for test runs. |
|
|
SmallCoccinelle
|
bc6d04dc2a
|
Remove new-from-rev from the linter (#1783)
* Fix all revive warnings in the code base All of these are of the form ``` var Identifier Type = Expr ``` where the `Type` is known from the output of `Expr` and can be omitted as a result. * Handle unchecked errors * Remove new-from-rev Since the project passes all linter checks now, including older revisions, we can remove new-from-rev. While here, reorder the linter config file, and move the enabled linters up and settings down. * Fix failing test cases Studio & Performer export tests use local time rather than UTC. This fixes the test cases so integration test passes. |
|
|
SmallCoccinelle
|
a2cce0ba77
|
Add golangci-lint workflow (#1759)
* Add golangci-lint workflow * Add a bit more lenient linter timeout 1 Minute isn't always enough, so bump to 3. * Document golangci, add make target Document how to get golangci-lint in the README file. While here, provide a QOL target in the makefile for the linter, and make it part of validation. * Introduce .golangci.yml This is the default golangci-lint configuration file location. Use it. Move configuration into the yaml file, and enable the default set of linters; we know we pass most of those. * Add gofmt and revive to golangci-lint Read the golangci-lint source code to figure out the configuration format. Copy the configuration from `revive.toml` into the linter configuration. * Do not set simplify on gofmt The project currently runs without simplify. So for consistency, don't make that a requirement for the linter. * Add new-from-rev Older issues should not be considered a failure for new PRs and issues. Use new-from-from to make the current develop as the point-in-time for when we consider errors. Once in the tree, we can go and fix the older errors in separate patches, taking a little bit at a time. * Move to golangci-lint Rewrite the way we run targets in the makefile, so it is split between frontend and backend. Use the frontend build steps in build.yml Update README to reflect the new world order. * Remove check-gofmt.sh The tool now runs as part of golangci-lint, in particular through the 'validate' target in the Makefile. * Remove targets for golangci-lint Fold these targets into the `lint` target. While here, update README. |