Every generator route was reachable by any account that could log in:
the /gen and /db routes were in CasbinExclude, and the /sys/tables
routes were mounted without AuthCheckRole. Any user could read and
change any table's configuration and, in dev mode, generate files
from it.
All thirteen now go through AuthCheckRole. admin is let through as
before; another role needs the generator's menus, whose APIs the
previous commit binds and grants. The captcha, registered alongside
them, stays public.
The package, table and business names a table's configuration carries
become directories and file names, and nothing on the server checked
them; the config page's patterns were the only check.
Each is now checked against what it becomes: a package name is
lowercase letters and digits, a table name letters, digits and
underscores, a business name an identifier starting with a lowercase
letter. None allows a dot or a separator, and none is stricter than
the config page. The check runs on import, where a list holding one
bad name is refused whole, on save, and again before generating, so a
configuration saved before this check existed is refused rather than
trusted.
The generator joins a table's package, table and business names into
the paths it writes, and wrote them with os.MkdirAll and os.WriteFile.
A name carrying "..", or a symlink under the target directory pointing
elsewhere, took the write wherever it led.
Every file is now written through os.Root: backend files under the
working directory, frontend files under gen.frontpath. A path that
resolves outside its root is refused, symlinks included, and the
request reports the write failure without writing anything outside.
A table whose columns are all its key and ones the framework fills in
got an add button and an edit link opening an empty form. The page now
leaves out both, the dialog, useForm and the API calls only the form
used. A page with anything to enter is generated byte for byte as
before.
The config page hid both columns along with the ones the framework
fills in, so a generated page could not list or filter by when a row
was created or last changed without editing sys_columns by hand. They
are now shown; id, create_by, update_by and deleted_at stay hidden.
The templates already keep both out of the create/edit form.
The test imports a table, ticks list and query on created_at through
the config page's own endpoints, generates it, and filters the
generated list with the value the generated date picker sends.
dropFixture soft-deleted the sys_tables rows it cleaned up. The config
page's business-name check counts soft-deleted rows, so a second test
importing the same table could not save its configuration. The delete
endpoint removes these rows with Unscoped; the fixture now does too.
v2.11.0 makes pkg.FileCreate return its error instead of ending the
process when the file cannot be created. Two comments in the code
generator described the old behaviour in the present tense.
Preview, GenCode, GenApiToFile and GenMenuAndApi read a table's
configuration with tab, _ := table.Get(...), so an id that matched nothing,
or a failed query, carried on with an empty table. GenCode and Preview then
refused it as having no primary key, and GenMenuAndApi went on to seed menus
and APIs for it.
Each now answers with the lookup's error.
Preview, NOActionsGen and genApiToFile assigned each template's Execute
error to err and never read it - 19 times - so a template failing part-way
left a truncated file behind it and the request reported success. The
files were written through pkg.FileCreate, which returns no error at all:
when it cannot create a file it closes a nil *os.File and calls
log.Fatalln, ending the server process. Four of the directories it writes
into were created with _ = pkg.PathCreate(...).
Every template now renders into memory first; if any fails, nothing is
written and the response names the template. Files are then written
through writeGenerated, which creates the directory and returns any error
instead of exiting. NOActionsGen and genApiToFile report whether they
succeeded, so GenCode and GenApiToFile no longer follow an error response
with "Code generated successfully".
SysTables.Create and Update wrote each column with _, _ = …, so a column
the database rejected was dropped while the request reported success. And
because neither ran in a transaction, returning that error alone would not
have been enough: the table row and every column before the failing one
were already committed, leaving a table the config page lists with columns
missing and code generated from fewer columns than the table has.
Both now run the table and its columns in one transaction and return the
first column's error, named. Create rolls back to nothing; Update leaves the
table and every column as they were. Both callers already answer an error
with a 500.
The generator had no test that compiled what it writes; test/gen_test.go is
commented out. This one goes through the real path end to end: fixture
tables in MySQL, the import handler, GenCode writing its files, the files
compiled into this module's own packages through a -overlay, and a CRUD
test run over HTTP against the generated handlers.
The shapes are an int key named id, an int key named otherwise, a string
key, no key and a composite key. The last two must be refused with nothing
written. For the rest: read and update through the path parameter, a
missing key neither found nor created by an update, a key of "1=1" read as
a value, and delete by key.
The importer only reads MySQL, so the test needs GO_ADMIN_TEST_MYSQL_DSN;
without it the test skips locally and fails under CI. It also asserts each
generated CRUD test actually ran, since a -run pattern that matches nothing
exits 0.
The templates address a row by a single key: one field on the model, one
path parameter, one value per row in a delete. A table with no primary key
rendered a model whose GetId returned an empty field name, and a composite
key was silently narrowed to whichever of its columns the importer read
last. Both wrote files that break the build of the whole module.
Preview and NOActionsGen now check first and return an error naming the
table and, for a composite key, its columns. Nothing is written.
jsonFieldPattern copied businessName's rule (^[a-z][A-Za-z]+$: at least
two letters, no digits) on the theory that jsonField should tighten to
the same identifier shape. That does not hold: businessName is typed
by a person on genInfoForm.vue, so a strict pattern is a reasonable
guardrail on human input. jsonField is computed by the importer from
the column name (sys_tables.go's namelist/JsonField loop) - nobody
types it, so the same pattern only rejected names the importer
legitimately produces: a single-letter column ("x") or one whose last
segment ends in a digit ("address2", "a1") both collapse to a single
camelCase word with nothing left to re-capitalize, and both failed the
old check.
The blast radius is wider than "this one column can't be edited":
validateAndSanitizeColumns runs over every column on every Update, so
a table that merely contains one such column could not save any
config change at all, including edits with nothing to do with that
column.
Relaxed to ^[a-z][A-Za-z0-9]*$ - any legal JS/TS identifier starting
with a lowercase letter. Still rejects what has to be rejected: empty,
whitespace/punctuation, and leading-digit names, since those cannot be
unquoted object keys in the generated interface/lang file at all.
Uniqueness and the expression-content check on defaultValue are
unchanged - defaultValue is genuinely user-typed (F6's config page),
so tightening it was the right call to begin with; this was the only
place a human-input rule had been copied onto machine-generated data.
Verified against the real import path, not hand-typed jsonField values:
built a table with columns id/x/address2/a1 in a fake information_schema,
ran it through the real SysTable.Insert, confirmed the importer computes
exactly jsonField x/address2/a1, then submitted an update through the
real SysTable.Update changing only tableComment (nothing about those
columns). Confirmed red first - 500, "jsonField 格式不合法：\"x\"" - a
change unrelated to any of the three columns was rejected solely because
they existed on the table. Restored the fix - 200, "修改成功".
MLTBName (table_name with underscores turned to dashes, e.g. "user_profile"
-> "user-profile") is a gorm:"-" field - table.Get never fills it in, the
caller has to. NOActionsGen has done so since it existed; Preview never
did, so every template's import path that reads it
(from '@/api/{PackageName}/{MLTBName}' in vue.go.template, present in
both the pre-Vue3 template and F4's rewrite) rendered with the module
segment missing - "from '@/api/admin/'" - in the preview dialog only.
The real generated file was always correct.
Pre-existing, not introduced by this PRD, but worth fixing now: F3/F9
added two more Preview panes (the language packs) on top of an assumption
- that Preview's output stands in for what NOActionsGen actually writes -
that was never true for this field. Checked the rest of gen.go for the
same "only set on the write path" shape; MLTBName is the only one -
every other field Preview's templates read comes straight off the
sys_tables/sys_columns rows table.Get already loads.
Verified with a throwaway harness driving Gen.Preview through a real gin
Context and sqlite-backed db, parsing the JSON response and reading back
template/vue.go.template's import line. Confirmed red first (temporarily
removed the added line): "from '@/api/verify010/'". Restored it: "from
'@/api/verify010/verify-widget'".
vue.go.template (F4) only had a flat fallback for an unconfigured
colWidth: 110 for datetime, 120 for everything else. R2 asks for a
precise per-type inference instead - varchar(n) tiered by n, tinyint(1)
narrower than a general integer, text/blob wide - sized so a typical
table's list columns land inside go-admin-ui's ~580px text-column
budget. text/template cannot parse "varchar(255)" itself, so this has
to run in gen.go before the template executes, not in the template.
Judgment is columnType, not goType, same as F1/F2's earlier reversal
(API契约.md §1.1): sys_tables.go:323-338 gives every non-primary-key
int/tinyint/bigint/decimal column goType "string", so goType alone
cannot tell a boolean flag from a bigint from a name column.
InferColumnWidth(columnType string) int is exported and pure per
测试用例.md §2.5's own request, so it can be pinned with an exact
input/output table rather than only asserting "the rendered page
happens not to overflow" - see column_width_test.go, including the
tinyint(1)-vs-tinyint(4) and the varchar tier-boundary cases.
applyInferredColumnWidths only touches a column already at the 0
sentinel, wired into both Preview and NOActionsGen ahead of the
template execute calls; a column already configured (by the user or F6)
is left untouched, also pinned by a test.
Verified against the real NOActionsGen call path (not just the unit
test): generated a table mixing every branch (tinyint(1), datetime,
decimal, varchar at three tiers, text, and one pre-configured column)
and printed the resulting tab.Columns[i].ColWidth after generation -
every value matched InferColumnWidth's own table, and the
pre-configured column's 333 was left untouched.
That bind-and-save path had zero field-level validation (API契约.md
§1.2/§2.1, decision D6), which left four ways to silently corrupt the
generator's own metadata or the language packs it writes:
- jsonField had no format rule at all, unlike tableName/className/
businessName, which all have a pattern check. A jsonField with a
dot, a leading digit, or anything not matching lower camelCase
still saved, and would land verbatim as a key segment in
gen/{PackageName}/{BusinessName}.ts.
- jsonField had no per-table uniqueness check. Two columns retitled
to the same jsonField overwrite each other's generated i18n key
with no warning.
- businessName had no cross-table uniqueness check within a
packageName. Two tables sharing one write the same language pack
path and the second's generation silently clobbers the first's.
- defaultValue is spliced into the generated defaultModel() as a
literal, never evaluated (API契约.md §2.1) - so content shaped
like a function call or block does not do what it looks like it
does, and previously saved without complaint either way.
colWidth is handled differently on purpose: API契约.md §2.1 says an
out-of-range value should fall back to the inferred width rather than
be rejected, so it is reset to the 0 sentinel in place instead of
failing the request - same outcome as if it had never been set.
G10 (colliding with the built-in admin/* i18n namespace) is not one of
the four: D9 already moved generated keys into their own gen/
namespace, so that collision no longer exists.
All four checks and colWidth's sanitize-in-place path are covered by
sys_tables_validate_test.go. Confirmed red first: swapped in a no-op
stand-in for both validators and reran - 13 sub-tests that should now
be rejected or sanitized passed straight through instead (jsonField
format x5, jsonField uniqueness x1, colWidth range x2, defaultValue
expression x4, businessName uniqueness x1). Restored the real
implementation and reran - all green.
Updates(&e) uses GORM's struct form, which skips zero-value fields -
but 0/"" is exactly the sentinel PRD 010 F1/F2 chose for "unconfigured"
(数据库变更.md §1.1). A caller resetting colWidth or defaultValue back
to that sentinel was therefore silently ignored: the row kept its old
value, the API reported success, and reopening the edit form showed the
stale number/string again.
Not fixed by switching to Select("*") - that would also start writing
this struct's other zero-valued fields (Sort, the Pk/Required/... bools),
none of which are part of this change. A second, map-form Updates
scoped to just these two columns writes the sentinel without touching
anything else; map-form Updates does not skip zero values.
TestSysColumnsUpdateClearsSentinelFields pins the regression: seed
150/"active", update to 0/"", read back and assert it stuck. Confirmed
red before this change (got 150/"active" back) and green after.
Add lang-zh.go.template and lang-en.go.template, one key per column
keyed by JsonField, valued from ColumnComment (R3) with a fallback to
JsonField when ColumnComment is blank (G11) - both locales resolve to
the same zh-CN text today since D3 rules out machine translation and
go-admin-ui's parity test rejects blank values.
NOActionsGen writes both files to lang/{locale}/gen/{PackageName}/
{BusinessName}.ts, two levels deep so go-admin-ui's gen-namespace.ts
glob (./*/*.ts under each locale's gen/) picks them up; nesting under
PackageName (rather than a flat gen/{BusinessName}.ts) avoids two
tables in different packages silently overwriting each other's
translations, since BusinessName has no uniqueness check.
Output has to be single-quoted with no trailing comma to satisfy
go-admin-ui's eslint config, which text/template's builtin `printf "%q"`
does not produce, hence the small parseGenTemplate/singleQuote helper
shared by the two templates.
Verified end to end: generated a real table's output (including a
blank-ColumnComment column, to exercise the G11 fallback), copied the
two language packs into go-admin-ui and ran vue-tsc --noEmit and eslint
against them - both clean, comma-dangle/quotes included.
Rewrite template/v4/js.go.template (renamed to ts.go.template) to emit
TypeScript matching go-admin-ui's src/api/demo/product.ts: an interface
per row and per query params, typed CRUD functions, and a del{Class}
that takes Id[] like useRemove expects. GoType maps to number for
int/int64/float32/float64 and string otherwise, since go-admin-ui's
request() types the {code,data,msg} envelope rather than the payload.
Preview's response map key changes from template/js.go.template to
template/api.ts.template so its Tab label matches the new content;
NOActionsGen writes the file with a .ts extension.
Verified end to end: generated a real table's output, copied it into
go-admin-ui and ran vue-tsc --noEmit and eslint against it - both clean.
Back the code generator's Vue 3 template migration: col_width lets R2's
column-width inference be overridden per field, and default_value lets
R1/A6's "unconfigured rows still generate a usable page" guarantee hold
for generated forms. Both use a sentinel default (0 / "") rather than
NULL so "unconfigured" has exactly one representation - see
docs-prd/010-代码生成器前端模板迁移Vue3/数据库变更.md §1.1.
The migration and the model change land together: AddColumn reads the
column definition off tools.SysColumns's own gorm tag, so splitting them
across commits would leave one of them failing to compile.
`gofmt -l` listed 26 files. Seventeen of them were missing the newline at the
end of the file; the rest are indentation that used spaces where the file uses
tabs, a handful of call sites written `f(a,b)`, and the doc comment spacing
gofmt has rewritten since 1.19 (`//X` to `// X`).
Nothing here changes behaviour: `go build ./...` and `go vet ./...` are clean
and `go test ./common/...` passes, which is the half of the tree these files
are concentrated in.
Only the files gofmt named are touched, so the diff reads line by line rather
than as a reflow of the whole repository. `gofmt -l` is now empty, which is the
precondition for gating it in CI -- worth doing, but a separate change.
The generator's import reads its comma-separated table list with
c.Request.FormValue("tables"), which on a request declaring itself as JSON reads
the URL query and nothing else. go-admin-ui v3.2.0 began sending that list in
the body, so the handler saw an empty string, asked information_schema for a
table named "", and every import failed with "table name cannot be empty！" —
on a fresh installation that is the first thing the generator is asked to do.
tablesToImport reads the query first and falls back to the body, so a front end
sending either works against this server. It also drops blank entries:
splitting "" yields one empty name rather than nothing at all, which is why the
old code reached a database query at all before failing.
The front end sends the list in the query again on its side; this half is what
lets an installation already running v3.2.0 recover without changing it.
The string was spelled out at each site that raises it, and once more in the
test file that asserts on it. A test holding its own copy cannot tell the
difference between the handler answering something else and the message having
been reworded: it goes on asserting a string the server no longer sends, and
goes on passing.
The three copies in app/other/models/tools are left alone; they are raised from
a different layer and nothing asserts on them.
newEngine takes the method, path and handler, so a second test file does not
have to restate the sqlite connection, the driver override and its cleanup, the
CustomError middleware and the two context keys. serveJSON does the same for
running one request and decoding the envelope.
Nothing about what is asserted changes; newColumnListEngine and columnListMsg
keep their names and their callers.
The warning said to set application.mode and stopped there. Following
that on a running process does not close anything: buildRouter has one
call site, in run(), and route registration is on no phase and no reload
callback, so a configuration reload moves the mode and leaves the routes
exactly where they were.
The reader is then worse off than before they acted. The mode now says
prod, GenWriteRoutesEnabled agrees, and the endpoints are still served -
so the one thing they could check to confirm the fix reports success
while the exposure is untouched, until something restarts the process.
A test pins the gap rather than the prose: build under dev, move the
mode to prod, and the routes are still in the engine. It fails if
registration ever becomes dynamic, which is the change that would make
the new sentence wrong.
That test degrades differently from the others - making it fail means
rewriting registration, not weakening it - so what was checked instead
is that it cannot go vacuous. Both of its premises are guarded: with the
gate always refusing it reports building under dev without the writing
routes, and with the gate always allowing it reports the predicate still
allowing prod. Neither failure can be mistaken for the assertion passing.
Raised by Copilot on #917.
registeredRoutes set config.ApplicationConfig.Mode and gave it back with
t.Cleanup, which runs at the end of the test rather than at the end of
the helper. Everything the caller did after the call therefore ran under
the mode the helper had been asked about, not one the caller chose.
Nothing was wrong yet: the one caller that reads the mode afterwards
sets it itself, and the two cleanups happen to unwind in an order that
leaves the right value. Both of those are accidents, and neither is
visible at the call site.
A defer inside the helper makes the borrowing end where it starts. The
doc comment said the mode was put back before returning while the code
did not, so that is now true rather than aspirational.
The counter-proof is the reason this has a test of its own: with
t.Cleanup back in place TestRegisteredRoutesRestoresTheModeBeforeReturning
fails and nothing else does, which is what a leak this quiet looks like
when something is actually watching for it.
Raised by Copilot on #917.
Three of the code generator's endpoints do not read. /gen/toproject
writes seven Go and Vue source files onto the host, one of them under
the path gen.frontpath names; /gen/apitofile writes a migration;
/gen/todb inserts menus and APIs. All three are GET, and all three are
listed in CasbinExclude - which AuthCheckRole skips - so Enforce never
runs for them. Any account that can log in could call them, on every
deployment.
They are now registered only where application.mode is dev or demo. dev
is the shipped default and is where the generator is meant to be used.
demo keeps them because demo mode already has a better answer than a
404: DemoEvn refuses these three by name and explains itself, which is
what the demo is for. test and prod get nothing, and so does a process
whose mode was never set.
This does not make the endpoints safe where they exist; it stops them
existing where nobody should be calling them. A host left on the shipped
dev is still open, which is why the next commit says so at start-up.
CasbinExclude is left alone on purpose. Taking the three off that list
would make them require a permission no existing deployment has granted,
so every non-admin user would start getting 403 from a tool that worked
yesterday. That is a migration, not a guard, and it belongs with a
release that can carry one.
Four counter-proofs, each red on the test that names the behaviour and
green everywhere else: a gate that always allows fails test/prod/unset
only; a gate that always refuses fails dev/demo and takes
TestEveryRouteDemoModeRefusesStillExists with it; moving a read-only
route inside the gate fails the reading test; and spelling the condition
at the registration site instead of calling the predicate fails the
agreement test, which is what keeps that test from being a tautology.
Two pre-existing deviations: a space before the comma in
sysNoCheckRoleRouter's parameter list, and no newline at end of file.
Separated from the change that follows so its diff is only the change.
DemoEvn decided by HTTP method: GET and OPTIONS through, everything else
refused. Three of the code generator's routes are registered as GET and write
anyway - two emit Go source files onto the server's filesystem, and the third
inserts menus, APIs and casbin rules into the database. They sit in a group
whose own name says it does no role check, and a demo deployment lets anybody
log in. So on the demo host any visitor could write to the machine and to the
database, and the one that writes menus had in fact been used: three generated
SysCasbinRule entries is how this was noticed.
The guard now also looks at the matched route. The method cannot answer the
question - whether a request changes anything is not something the verb reports
truthfully here - so the three are named, as gin route patterns, which is what
Context.FullPath returns and how CasbinExclude already spells them.
Changing them to POST would be the better shape and is not this change. sys_api
records an endpoint by method and path and the casbin policy follows it, so
flipping the verb needs a migration and a policy resync; until both land, every
existing deployment would start answering 403 to a role that could use the
generator the day before.
The read-only half stays reachable: preview, the table tree, and the two
database listings. A demo host that cannot demonstrate the generator is as
broken as one that lets visitors write to it - refusing too much is the same
defect facing the other way, and there is a test for that direction too.
Half of the general hole is closed and the other half is written down. The
closed half is a test beside the route registrations: it builds the generator's
routes, enumerates them, and fails if any entry in the guard has stopped being
a real route, so renaming one turns the list red instead of quietly making it
match nothing. It lives there because common/ may not import app/ - which is
also why the guard cannot check its own list from where it is. The open half is
that no static check can tell a handler that writes from one that reads, so the
next GET that writes has to be added by hand. The comment says that rather than
leaving the impression the class is covered.
application.demomsg was configuration nothing read. The message was hard-coded
in the middleware, and the demo host's configured string happened to be
identical, so the setting looked like it worked and never had. It is read now,
with the old string kept verbatim as the fallback, so a deployment that never
set it is answered exactly as before.
This covers demo mode only. On a deployment that is not a demo those three
routes remain in CasbinExclude and stay reachable by any authenticated user
whatever their role; that is a separate decision and is not touched here.
The manifest in this repository had no probes at all. A pod was sent traffic as
soon as its container was running, whether or not the database it needs was
reachable, and it was stopped with whatever grace period Kubernetes defaults to
rather than one chosen against what this process actually spends shutting down.
It now mounts both probes, at the endpoint that answers each question:
readiness at /ready, which fails while a dependency is unreachable, and
liveness at /health, which is a bare 200 because restarting a process whose
database is down turns one outage into a crash loop. Both skip the rate
limiter, which is why that had to land first.
timeoutSeconds is 3, not the default 1. The handler allows its checks two
seconds, so at the default a database answering in 1.2s would be recorded as a
failed check while the handler was returning 200 - the probe would be failing on
the orchestrator's stopwatch, not on its own. The comment beside that constant
said the constraint was the polling period; the constraint is the per-check
timeout, and it is now written down correctly.
terminationGracePeriodSeconds is 30, against a shipped budget of 0 + 5 + 3.
Raising drain means raising this too, in the same commit; the check that
notices when somebody does not arrives two commits from here.
replicas stays at 1, and the comment says why that makes the drain window worth
nothing: there is nowhere to send the traffic this pod stops taking. Raising it
needs one more change than the number - the volume is shared by every replica
and the log path lives on it, so a second pod would append to the same rotating
file. The reason not to raise it is not the one the review assumed: the claim
was that the PVC is ReadWriteOnce, and it is not, it is ReadWriteMany on nfs-csi.
There is no preStop hook. How long one should sleep depends on how fast the
thing in front removes this instance, which the repository cannot know, and a
manifest carrying both a preStop sleep and a drain window is the double-counting
trap - the budget would be spent twice and the start-up line would report half
of it.
/ready has failed from the moment shutdown begins since the readiness probe was
added, and the order it does that in is right: reversed, the state would be
reported after the connections were already cut. But order alone does not
produce a window. Nothing waited between the flip and Shutdown, so the two were
microseconds apart, and a poller on a multi-second interval never saw the 503 -
it saw a refused connection, which is the thing the probe was supposed to
avoid. Polling a container through a SIGTERM on the demo host recorded exactly
that: 200, then connection refused, and no 503 in between.
extend.shutdown.drain is that wait. The process keeps serving normally for it -
answering requests, not refusing them, because refusing them would move the
outage earlier rather than avoid it - and only then closes the listener.
It is zero by default, so nothing changes for a deployment that does not ask
for it. That is not timidity: the budgets are spent one after another, and a
non-zero default would push every existing shutdown closer to the orchestrator's
grace period, where being cut off part-way through the cleanup callbacks is
worse than never draining at all.
Keep-alive is switched off with the flip. The server keeps connections alive
until Shutdown sets shuttingDown() itself, so without this the pooled
connections a balancer holds would sit untouched for the whole window and be
cut at the end of it anyway - the cost of the window without its benefit. This
is the switch Shutdown flips, moved earlier by the window's length.
The signal disposition is restored after the window rather than on the first
signal. Before there was a window, the interval where a second signal killed
the process outright was only reachable while a cleanup callback hung; putting
a multi-second wait inside it would have made every ordinary shutdown
interruptible for the length of the drain. A second signal during the window is
taken by the channel and ends the window early instead - somebody sending
another kill wants this over with sooner - and the escape hatch comes back the
moment the window does.
What the window is worth depends on who removes this instance. A balancer that
polls /ready acts on the 503 and needs the window to cover its check interval
times its failure threshold; a Kubernetes Service withdraws the endpoint when
the Pod is deleted, concurrently with SIGTERM and regardless of what the probe
returns, and there the window covers the delay in that removal reaching every
node. The three comments that used to say a balancer "has a chance to" take the
instance out said it without either qualification, which is how a claim comes to
be repeated after a live test has refuted it.
The subprocess test polls the real probes on a connection it opens after the
signal - a reused one can be served after the listener is closed, which would
let this pass against a shutdown that had already broken it - and asserts on the
draining answer in the body, not on the status code. With no database the status
is 503 from start-up, so a status-code assertion would hold even with
BeginDraining deleted. Two window lengths, because one proves only that
something takes that long.
The limiter is installed on the engine and the probes are routes like any
other, so above the threshold they are answered with 429 too. Point a liveness
probe at one and the failure mode writes itself: traffic crosses the threshold,
the probe collects three 429s, the kubelet restarts the container, the capacity
that was already short gets shorter, and the instances that are left are pushed
further past the threshold. The limiter working exactly as designed is what
kills the pod.
It is the argument common/health already makes about restarting a process whose
database is unreachable, applied to load: turning one outage into a crash loop
is not an improvement on the outage.
Nothing points a liveness probe at these routes yet. The manifest that will is
two commits away, and this has to land first, because that manifest without
this change would be actively harmful.
The exemption wraps the middleware rather than teaching the limiter about these
paths. common/ may not import app/ - the contract check enforces it - so the
limiter cannot name routes that are registered over there. Wrapping it in the
command package, which imports both, is what keeps the boundary.
Naming those routes needs them exported, so the group prefix and the two paths
become constants and the router function becomes RegisterMonitorRouter. That
also gives a test something real to mount: a probe asserted against a
re-implementation of itself is a test of the copy.
The check that the middleware never runs is separate from the check that the
answer is not 429, because a probe can produce a 429 on its own. What has to be
true is that the request never reached the limiter.
Three comments said readiness failing before the server stops accepting gives
a load balancer the chance to withdraw the instance before its connections are
cut. Nothing between the two lines makes that possible: BeginDraining is
immediately followed by the shutdown, and a poller on a multi-second interval
never observes the flip.
On Kubernetes the endpoint is withdrawn when the Pod receives a
deletionTimestamp, concurrently with SIGTERM and independent of what the probe
returns, so the probe result is not the mechanism there either.
The order itself stands - reporting the state after the connections are cut is
worse - so the comments now say the order is necessary and not sufficient, and
that a window needs a configured delay that does not exist yet.
/health returned 200 without asking anything. Whatever it was meant to say, an
orchestrator reading it learned only that a process was accepting connections.
The two questions are not the same one, and the answers differ:
- /health stays a bare 200. It answers "should I restart you", and a process
whose database is unreachable does not want restarting - that turns one
outage into a crash loop and discards the connection pool, the cache and
every request in flight on the way.
- /ready is new. It answers "should I send you requests", fails while a
dependency is unreachable, and fails from the moment shutdown begins.
That last part is what the life-cycle phases bought. BeginDraining sits next to
BeginShutdown, before the server stops accepting, so a load balancer is told to
stop sending while this instance can still finish what it holds. Reversed - and
that is where it was - the connections are cut first and the probe reports it
afterwards.
The queue is deliberately not checked. Nothing on AdapterQueue answers "are you
reachable" without publishing something, the memory backend cannot fail, and a
queue that is down degrades logging rather than stopping requests: a reason to
alert, not a reason to leave the pool.
The cache probe writes and reads back rather than only reading. A cache that
answers "miss" for every key - a client pointed at the wrong server - is
indistinguishable from a healthy one on a read alone.
Every check runs behind a recover, and that is not defensive habit. The test for
"nothing configured" found the reason: GetCacheAdapter builds a wrapper around
whatever is configured and returns it even when nothing is, so the value is not
nil, the cache inside it is, and Set dereferences it. A nil check cannot see
that, and GetQueueAdapter behaves the same way. Whatever the cause, a probe is
the last thing that should be able to take the process down - the caller is
asking whether this instance is well, and killing it is the wrong reply.
The counter-proof compiles and fails: without the recover, the unconfigured case
panics rather than reporting two failed checks.
Four modules each called AuthInit and built their own instance, so which
one Runtime handed back was decided by whichever module initialised last.
The JwtToken key was also registered as an unbound method expression,
which GetHandlerFunc's type assertion can never match - the key was
registered and unusable at the same time.
The instance is now built once here and registered as a bound closure.
Modules read it back through GetAuthMiddleware, which is fatal rather
than nil when called before InitMiddleware has run: a process without a
JWT middleware should not reach the point of serving a request.
Only one call site needs the instance itself rather than the handler
(admin's /login, for LoginHandler); the thirty-odd MiddlewareFunc() call
sites are unchanged.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
thirdUpload dispatched on the source parameter and then built the same
zero-value ALiYunOSS in both branches, so source=3 could not have
reached qiniu even with credentials.
Neither branch had credentials to use. OXS.Setup is the initialisation
path and nothing in the repository called it, and no configuration field
existed to fill. The store is now taken from extend.fileStore, and a
provider that was not configured says so rather than producing the
provider's own complaint about an empty bucket name.
The two handlers passed errors.New("") to e.Error, discarding what
actually went wrong; they now pass the error.
Every import of the module changes, not only the seven packages that
moved out of sdk/pkg: Go requires the major version in the path from v2
on. Both happen in one pass —
go run github.com/go-admin-team/go-admin-core/tools/coreupgrade@v2.0.0 -w -v2 .
go mod tidy
— which is the command the release notes give, run here as a consumer
would run it. 210 imports across 95 files.
The compatibility shims this used are gone in v2, so the paths that
moved had to move: sdk/pkg/captcha, sdk/pkg/jwtauth and its user
package, sdk/pkg/response and sdk/pkg/casbin.
The count of unformatted files is unchanged at 34, none of them touched
by this: the tool reformats a file only if it was already gofmt clean,
so a migration cannot disappear into whitespace.
The exclusion list was a subquery against `$GenConfig.DBName`.sys_tables,
so it named the schema by hand. Generating from a schema that is not the
one holding sys_tables made the whole query fail, and because the
subquery read the table directly it also counted soft-deleted entries:
deleting a generator entry never handed its table back.
Read the registrations through the model on this connection instead. An
empty list skips the clause - NOT IN (NULL) is unknown for every row,
which would leave a fresh install with nothing to generate from.
pkg.Assert panics when its condition is false, so pkg.Assert(true,
"目前只支持mysql数据库") is a no-op. On postgres or sqlserver the code
generator did not report that it needs MySQL: DBTables returned an empty
list with a nil error, and DBColumns ran its query on the zero-value
*gorm.DB left over from the branch that never assigned, which is a nil
dereference rather than a message.
Assert the driver up front instead of asserting a constant in an else,
which also removes the placeholder *gorm.DB the fall-through relied on.
DBColumns.GetPage had no guard at all and gets the same one.
pkg.Assert panics when its condition is false, so
Assert(TableName == "", "table name cannot be empty") rejected every
request that carried a table name and let the empty one through. The
model layer repeated the inversion with if TableName != "" { return
error }, so either one alone was enough to break the endpoint.
Flip both, and hoist the model guard out of the mysql branch so it
matches GetList ten lines below, which had it right all along.
zhangwenjian