core's contract says what the four phases promise. This says which line of
cmd/api/server.go each of them is, which is the part an application author
cannot read out of core.
Three things are recorded because getting them wrong is silent:
- BeforeRouter is not the before registry. Those callbacks run from
runStartupHooks, which is after initRouter has built the engine; the phase
is before it. The two are one line apart in the same function and describing
them as equivalent is the mistake this paragraph exists to prevent.
- AfterResource runs again after every configuration reload, so a callback
there is idempotent with respect to a resource rather than doing nothing
the second time. The queue consumers are the worked example, in both
directions: a new adapter must get consumers, the same adapter must not get
them twice. Identity has to come from where the resource is created -
GetQueueAdapter and GetQueuePrefix build a fresh wrapper per call, so two
of them never compare equal however often the adapter underneath changed.
- Consumers must be registered before the queue is started, and which
implementation is behind the interface decides whether that matters:
QueueConfig.Setup hands back queue.NewMemory when there is no redis
section, and that one does not care; a redis section reaches
LegacyQueueAdapter, whose Register cannot report a refusal. The table says
so rather than leaving "only on redis" as a claim. What is silent is the
registration alone - every publish afterwards is refused with ErrNoHandler
and logged at error level, so the symptom is missing log rows plus a lot of
noise, not a quiet nothing.
The third-layer table loses its "queue consumers are lost after a hot reload"
row, which is what AfterResource is for, and gains the honest replacement: the
four phases are the only mount points there are. There is no "after the routes
are installed, before the socket is listening".
AfterListen is described as the port being bound rather than Serve being in its
accept loop. Serve runs on another goroutine and may not have reached the first
Accept; what is true is that the bind returned, so the kernel is queueing
connections. A bind that fails produces no phase at all - the error returns
from run() and the banner never prints.
The per-tenant setup ended with `defer crontab.Stop()` on the line above
`select {}`. The select never returned, so the deferred call was unreachable
for the life of the process: the scheduler had never once been stopped. And
because setup never returned, the `for k, db := range dbs` loop in Setup never
reached its second iteration - with several tenant databases configured, only
whichever one came first out of the map ever got a scheduler at all.
Both fall out of deleting the select, which was blocking for nothing: cron.Start
is `go c.run()` and has never needed anything to hold the caller.
The stop becomes a BeforeExit callback. cron.Stop returns a context that closes
once the jobs already running have finished, so the shutdown budget has
something real to bound - and giving up on that wait leaves those jobs running
until the process exits, which is better than holding the whole shutdown open
for one job that will not end.
Startup moves from a bare goroutine in run() onto AfterListen. Two reasons: the
phase runs behind core's panic guard, which does not reach across a goroutine
boundary, so a panic while loading jobs used to take the process down; and the
jobs it starts can call the API, which is only true once the socket is
accepting. It can be synchronous now precisely because setup returns.
Tested where it can be: startCrontab is split out so a scheduler can be started
with no database in sight. The job runs every second; after RunShutdown, two
and a half seconds of silence is the assertion. The counter-proof - registering
no callback, which is what this commit replaces - compiles and reports "the job
fired 2 more times after shutdown".
There is one test, not several, because BeforeExit closes to further
registration once it has run; a second RunShutdown in the same binary would
find an empty registry and pass while proving nothing.
**The multi-tenant half has no test.** setup needs a *gorm.DB per tenant before
it reaches the line that was blocking, and this repository's CI has no database
- `make build` is CGO_ENABLED=0 with no sqlite tag. It is the same defect
though: the loop could not advance past a call that never returned.
setupQueue ended with `go queueAdapter.Run()`, and the three log consumers were
registered afterwards, from setup(). The contract implementations refuse a
registration once the queue is running - memqueue and the redis queue both
answer storage.ErrQueueAlreadyStarted - and Register cannot report it: it
returns nothing, which its own comment in core records as the reason the
interface is deprecated. Start first and register second, across two
goroutines, and the registration is dropped without the caller being able to
tell.
What follows is not quiet. No consumer group was created, so redis refuses
every later publish with storage.ErrNoHandler, and go-admin logs that at error
level from both call sites while the login and operation log rows are simply
never written. The silence is in the registration; the cost shows up on every
request after it.
Which implementation is behind the interface depends on the configuration.
config.QueueConfig.Setup returns queue.NewMemory directly when there is no
redis section - and that one does not care about the order, because its
Register just starts another consumer goroutine. Only a redis section reaches
storage.LegacyQueueAdapter, which wraps the contract implementation and
therefore refuses. So the defect is invisible in the default deployment and
shows up only where redis is configured, dropping the login log, the operation
log and the api check - the three things #892 was about.
The start therefore moves to the code that registers, and nothing starts the
queue but that.
The registration also moves onto AfterResource. It has to: a reload rebuilds
the adapter, and consumers attached to the one that existed at start-up are
attached to a queue nobody publishes to any more. Being on that phase means
running again on every reload, so the callback is idempotent with respect to a
given queue rather than "does nothing the second time" - registering twice on
the same queue would give every message two consumers and write every log row
twice.
Identity for that comes from common/storage, where the adapter is built, as a
generation counter. It cannot come from the accessors: GetQueueAdapter and
GetQueuePrefix build a fresh runtime.Queue wrapper on every call, so comparing
two of them compares two wrappers and never matches however many times the
adapter underneath has been replaced. A counter also keeps the comparison on a
uint64 rather than an `==` between two interface values, which would panic on
an adapter type that is not comparable.
Generation 0 means the configuration has no queue section, so nothing was
installed and the runtime hands back its own memory queue. That case still gets
consumers, because the registration this replaces was unconditional and
dropping it would stop the logs for anyone who commented the section out.
Two things fixed on the way past:
- `if q := sdk.Runtime.GetQueueAdapter(); q != nil { q.Shutdown() }` was
always true. GetQueueAdapter never returns nil - with nothing configured
the runtime falls back to its own memory queue and wraps that - so the
first start shut down the fallback queue before anything had used it. Only
an adapter this package installed is shut down now.
- config.Setup becomes bootstrap.SetupConfig, which is what announces
AfterResource, and announces it after the callbacks that build the
resources rather than before.
attachConsumersOnce is split out so the order and the once-ness can be checked
against a queue the test controls; neither can be read back out of a real
adapter. Four tests cover the ordering, both directions of the idempotency
rule, and the unconfigured case. Both counter-proofs compile and fail: calling
Run before the registrations reports each of the three as "came after Run", and
dropping the generation guard reports eight calls where four are wanted.
One honest limit: the counter-proof for the ordering makes Run synchronous.
The original arrangement started the queue on another goroutine, and a race
cannot be made to fail every time - which is the reason the order is enforced
by structure here instead of being left to be noticed in use.
A module can now register cleanup and have it happen. Until this commit the
process stopped serving and returned; anything a module had set up went down
with the process rather than being taken down.
BeginShutdown is said first, before anything is dismantled. Without it a
configuration reload arriving in this window re-runs AfterResource - rebuilding
the pool and the queue adapter and re-registering consumers - on top of cleanup
that has already run.
The cleanup runs whether or not Shutdown reported an error, which is the whole
reason that error stopped being fatal in the first place: Shutdown fails exactly
when connections were still in flight, and that is when there is most left to
take down.
The two budgets are spent one after the other, so what has to fit inside the
orchestrator's grace period is their sum. `docker stop` allows 10s by default
before SIGKILL; 5+3 leaves room to finish returning. Raising one without
lowering the other buys nothing.
Both halves are tested through the existing subprocess child, which now
registers a BeforeExit callback of its own:
- after a Shutdown that timed out, the callback still runs. Moving the call
into the success branch reports "the BeforeExit callback did not run after
a failed Shutdown".
- a callback that outlasts its budget is abandoned, not awaited. It sleeps
two seconds against a 300ms budget; RunShutdown reports the deadline, the
process exits cleanly inside one second, and the callback's own marker
never appears. Widening the budget to five seconds makes the test time out
waiting for the exit, which is what "awaited" looks like.
Both counter-proofs compile and fail.
Two phases are now announced from the command that serves traffic, so a module
can attach to them instead of being called by name from here.
The listener is opened by this goroutine rather than left to ListenAndServe,
which binds on the goroutine that serves. That mattered for the phase: a hook
on AfterListen is promised a reachable port, and with the bind happening out of
sight there was no way to keep that promise - "address already in use" surfaced
on a goroutine nobody read, after the banner had already announced the server
was up. It is now returned from run() and the process exits non-zero without
claiming anything.
AfterListen is announced synchronously. Moving it to a goroutine to save the
few milliseconds would let it overlap the shutdown, and on a fast SIGTERM the
cleanup callbacks could finish before the startup ones did.
What is left in the serving goroutine is still log.Fatal, deliberately: the
bind is no longer among the errors that reach it, so what remains is a serve
that failed after the port was taken, and carrying on would park the process on
<-quit with nothing serving. ServeTLS is the one case that can still fail
immediately, since it reads the certificate files - with ssl enabled a hook can
still run against a server on its way down. That is not a regression (the old
code printed the banner in the same situation) and it is not fixed here.
BeforeRouter is placed before initRouter, which is a different moment from the
before registry runStartupHooks drains: those callbacks run after the engine
has been built, not before it.
AfterListen is tested here, in one test rather than two because the phase seals
itself once it has run: a second test would find a closed registry and pass
while proving nothing. Both counter-proofs compile and fail - announcing on a
failed bind reports "AfterListen ran 1 times after a failed bind", and
`go RunPhase(...)` reports "ran 0 times, want 1" against the hook's own pause.
BeforeRouter's placement is not asserted in this commit. The test for it comes
with the buildRouter extraction later in this branch.
It carries the life-cycle phases and the shutdown registry. Nothing here uses
them yet - the wiring is the commits that follow, and keeping the bump on its
own means a bisect can tell "the dependency moved" apart from "the host
started calling into it".
The comment claimed bindings were collected as the body was walked so that a
registration only saw definitions above it. That is the single-pass design this
started as. The code does two passes - one to collect, one to report - and a
registration therefore sees every binding in the function.
That is the point rather than an accident: a `.Use` written below a route is
still part of the chain, because the chain is assembled before anything is
served. The price is that a name reused for two different things in one
function resolves to the last assignment, which the comment now says instead of
promising an ordering the code does not keep.
GetPermissionFromContext cannot fail. When no middleware put a *DataPermission
in the context it returns the zero value, and the zero value's DataScope is the
empty string - which is not one of the five scopes Permission recognises, so it
takes the default branch and fails closed. The query is handed `1 = 0` and
matches nothing.
The endpoint then reports "not found" or "no permission" for rows that plainly
exist, and only where enabledp is true. With data permissions off - the
repository default - Permission returns the query untouched and the missing
middleware costs nothing at all. A test suite and a CI that run on the default
cannot see it.
That is what happened to /api/v1/getinfo: it read the permission on a group
carrying only the JWT middleware, so every login on a deployment with data
permissions enabled ended in a 401 from the endpoint the browser calls
immediately after signing in, and went back to the login page. Three /sys-api
routes had the same shape.
The check matches a handler to the group it is registered on, through the AST
rather than through the text - a scratch grep for the same thing reported four
false positives from a comment that happened to contain the function's name,
and before that, a dozen from matching handler names across packages. Handlers
are keyed by package, type and method, so two SysUser types are two handlers.
Subgroups inherit their parent's chain, as gin does, and a `.Use` written below
a registration still counts, because the chain is assembled before anything is
served.
Either half is a fix and the message says both, because which one is right
depends on the route. A handler reading other people's rows wants the
middleware. A handler reading the caller's own row - id from the token - wants
no scope at all: DataScopeSelf matches on create_by, so scoping a self-read
rejects every user who did not create their own account. Reporting only "add
the middleware" would have turned /getinfo from broken into worse.
Five tests: the mistake, both fixes, subgroup inheritance, and a same-named
handler in another package. TestThisRepositoryIsClean covers the real tree, and
it is what fails on the commit before this one - four findings, all real.
Logging in on a deployment with enabledp: true ends on the login page. The
login itself succeeds - sys_login_log records it - and then /api/v1/getinfo
answers 401 "登录失败", which sends the browser straight back.
The query behind it reads:
SELECT * FROM sys_user WHERE sys_user.user_id = 1 AND 1 = 0 AND deleted_at = 0
The 1 = 0 comes from the data-permission scope. GetInfo asked for a permission
with GetPermissionFromContext, but the group this route sits in installs only
the JWT middleware - no PermissionAction - so nothing ever put one in the
context and what came back was the zero value. An unset scope is not one of the
five recognised ones, and since unknown scopes began failing closed rather than
silently matching every row, that zero value now means "match nothing".
The route was working by accident before, and only on deployments that enable
data permissions: the repository default is enabledp: false, where Permission
returns the query untouched. That is why the local suite and CI are both green
and the demo site is not.
Two different faults, so two different fixes:
/getinfo reads the caller's own row - the id comes from the token. A data
scope answers "whose rows may this user see", so there is nothing left for it
to restrict, and applying one is not a stricter version of the query but a
broken one: DataScopeSelf matches on create_by, and an account is created by
whoever added it, so a scoped self-read would 401 every user who did not create
their own account. It now goes through GetSelf, which does no scoping at all -
which is how GetProfile has always read the same row.
/sys-api is the opposite case. Its three handlers do read the permission, and
they are listing and updating other people's rows, so the middleware belongs
there and was simply missing. Added.
Those four endpoints were found by checking every handler that reads the
permission against the group it is registered on. The check reports four before
this commit and none after.
No test. Both paths need a *gorm.DB with sys_user and sys_role rows before they
reach the line that matters, and this repository's CI has no database - `make
build` is CGO_ENABLED=0 with no sqlite tag. What can be tested is the shape of
the mistake rather than its effect, and that belongs in tools/checksilent as a
rule of its own; it is not in this commit because a site that cannot be logged
into should not wait for it.
Moving the dial to just before the shutdown removed one flake and introduced
another: Shutdown only waits for connections the server has already accepted,
so calling it in the gap between the dial and the accept finds nothing to
wait for and returns cleanly. The test then fails on its own "this proves
nothing" guard - which it did, after passing once.
A ConnState hook closes both gaps deterministically. The connection is opened
late enough not to age past the five seconds net/http stops counting it at,
and the child does not proceed until the server has taken it off the
listener.
Ran five times in a row rather than once, because a single green run is what
made the previous version look fixed.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
net/http stops counting a StateNew connection against Shutdown once it is
more than five seconds old. The connection was opened when the child started
and the parent then waited for readiness before signalling, so on a slow run
the connection could age past that mark and Shutdown would succeed - and the
test would fail on its own "this proves nothing" guard rather than on the
behaviour it is there to pin.
Opening it immediately before the shutdown keeps the timeout deterministic.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The previous commit split arming from waiting so a caller could arm first,
wrote a comment saying a signal landing in between reaches the default
handler and kills the process, used it that way in the subprocess test - and
then left run() calling the combined helper after the whole readiness banner.
The window it warned about was still there in the one place that ships.
The signals are now armed before the server starts serving, and the wait
happens where it did. The disposition is restored right after the first
signal rather than deferred, so a shutdown that hangs can still be
interrupted by a second one.
waitForStopSignal goes away: run() was its only caller, and what was worth
keeping from its comment is now on armStopSignals.
Note that no test covers this ordering. The subprocess test drives
armStopSignals directly, which is what makes it a test of the mechanism
rather than of run(); moving the call back below the banner leaves it green.
Verified by reading the sequence in run(), not by a failing test.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
Three defects on one path, none of which could be seen from the code alone.
**SIGTERM was never registered.** signal.Notify listened for os.Interrupt
only, and Go terminates the process outright for a signal nothing handles.
`docker stop`, a Kubernetes pod deletion and `systemctl stop` all send
SIGTERM, so every line of the graceful shutdown below the wait was dead code
outside a terminal: measured on a real binary, SIGINT printed "Shutdown
Server ..." and "Server exiting" and SIGTERM printed neither.
**A stuck shutdown could not be interrupted.** quit is buffered and
signal.Notify stays armed after the first delivery, so further signals only
refill the buffer. That was harmless while SIGTERM went to the default
handler - it was the escape hatch. Registering it removes the hatch, so the
disposition is now restored once the first signal is taken, and a second one
kills the process the default way. Arming is split from waiting so a caller
can arm before it announces readiness; a signal in between reaches the
default handler, which is the very failure being fixed.
**A failed Shutdown skipped everything after it.** log.Fatal is an
unconditional os.Exit(1), and Shutdown reports an error precisely when
connections were still in flight - the moment the cleanup that follows
matters most. It is an error now, and the process carries on.
That failure is closer than it looks. net/http only treats a StateNew
connection as idle once it is over five seconds old, so a connection opened
shortly before the signal that has sent nothing holds the whole budget: with
the shipped settings.yml (readtimeout 1) the server closes it first and
shutdown takes 5ms, but with settings.demo.yml (readtimeout 10000) the same
connection made shutdown take 5.04s and exit 1, printing no "Server
exiting". The default configuration is what has been hiding this.
The wait and the shutdown are extracted so the subprocess tests can drive the
real functions against an empty http.Server: CI has no database, and none of
this needs one.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The note explained the empty-shim summary as the expected state "until the
contract packages are lowered into core". They are lowered, and the shims
exist - so a count of zero now means they stopped being aliases, or stopped
being here, which is the interesting case rather than the ordinary one.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The contract-shim-alias message built its suggested line from path.Base of
the import path, so it told the author to write
type ControlBy = models.ControlBy
in a file whose import is `contractmodels "…/sdk/contract/models"`. Every
shim in this repository aliases that import, so the suggestion never
compiled as written - in the one message whose whole job is to be pasted in.
qualifiedType already read the in-source identifier to resolve the import;
it now returns it.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
Widening menu-sort-overflow to see a contract MenuSpec made it fire on
app/admin/service/seed_test.go, on the case that asserts SeedMenus rejects
a sort of 900. The check was reading the proof that it works as a defect.
That is not specific to this one guard: menu-sort-overflow,
config-value-truncation, menu-id-collision and modeltime-mix are all about
a value that reaches a real database through a migration, a test fixture
reaches none, and every one of those guards needs a test that writes the
value it rejects. Skip _test.go in all four.
The two import and alias checks keep scanning tests - those are about the
dependency graph, where a test file's import is as real as any other, and
TestContractImportBoundaryCoversTestFiles already pins that.
Both directions are covered: a fixture is ignored, a real seed is still
reported.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The naming rule was documented; what happens when it is broken was not.
It now panics naming the offending file, which is worth saying out loud
because the alternative it replaced was silent: a name that is not a
timestamp used to register as its own version, and that migration would
never run and never report anything.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
Sort has an upper bound: sys_menu.sort is built as a tinyint, sqlite ignores
the width, and an overflow surfaces as Error 1264 partway through a migration
rather than as a rejected value.
MenuSpec carries no menu name, and the host synthesises one from the app code
and the spec code rather than using Code directly - two applications both
choosing "list" would otherwise share a keep-alive cache key on the frontend.
Nothing in the type says so, and every Seeder implementer would have to
rediscover it.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The sort-overflow check recognised a SysMenu literal from this repository's
model packages and nothing else. An application installed from outside cannot
reach that type - it describes the same row as a seed.MenuSpec and hands it to
the host's Seeder - so the check went quiet for exactly the author furthest
from the schema it protects.
Not hypothetical: this repository's own reference application shipped a Sort
of 200, past the tinyint sys_menu.sort is built as, and this check passed it.
sqlite ignores the width, so it would have surfaced first on a real install,
as Error 1264 partway through a migration with everything after it unapplied.
The check still cannot see an application in the module cache; that half is
the Seeder's runtime validation. This closes the half that is in the tree.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
Two of the three bullets on the contract surface disagreed with
docs/contract.md and with core's own. Registration is constrained by
ordering - it must happen before the startup hooks run - not by being
written inside `init()`; and the claim that core's setters take no lock
is not true of them. The check table gains the new alias rule.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The list of stable packages named four packages of this repository, on
the stated grounds that deduplicating app/demo's imports produces
exactly those four. That reasoning was wrong in the one direction that
matters: it sends a third-party author to depend on the host, and the
host is a fork that every user edits. `go-admin` is also not a
resolvable module path - it has no dot in its first element - so an
application cannot require it at all without a replace directive, which
is ignored outside the main module.
Rewritten around what core promises instead, and around a different
question: not "which packages does an application import" but "which
conventions fail without saying anything". Those are now spelled out
one by one, each with the mechanism that makes it silent - the response
envelope the frontend reads by `code`, the tenant-scoped connection,
`create_by` and the soft-delete marker, the data-scope middleware, the
transaction shape, and the `apps/` prefix a packaged application's menu
component must carry.
Also states two things the document was missing: installing an
application means trusting it with the host's database connection, at
the same level of trust as importing any other Go package - there is no
sandbox here and this does not pretend otherwise - and wiring an
application in touches two places, not one, where missing the second
means the migrations simply do not run, with no error.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
A shim of a core contract type written as `type X pkg.Y` instead of
`type X = pkg.Y` keeps the fields and drops the method set, so anything
embedding it stops satisfying the interfaces it satisfied before.
The compiler catches that only where the method set is actually
exercised. This repository exercises some of the contract types through
an interface and some not at all, so the ones it does not exercise
compile here and break in a fork or a third-party application - which
is the half nobody is watching.
The trigger is the right-hand side of the declaration rather than a
list of package names, so it covers whatever the lowering ends up
shaping without a list to keep in step. Until the contract packages
land in core there is nothing here to guard, and the summary says so
rather than letting the silence read as a pass.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The comment called Visible "0" "hidden by default" and then said an
administrator should not have to unhide the menu - which cannot both be
true. "0" is shown; every menu this repository seeds, including the demo
product menu that is visible on the demo site, uses it.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
grantToAdminRole does two independent things - it grants the menus to the
admin role and writes a casbin rule per api - and SeedMenus skipped the
whole call whenever the menu list came back empty.
An application is free to register apis with no menus: endpoints another
service calls, a webhook, a UI mounted somewhere else. Those installs wrote
their sys_api rows and then no casbin rule for any of them, so every one of
those endpoints was denied to everyone, admin included - from a migration
that reported success and left rows in the table to prove it had run. There
is nothing to look at afterwards that says what went wrong.
Guard on both lists instead, so nothing registered stays a no-op and apis
alone still get granted.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The host kept its own copy of the version-naming rule, byte-identical to
the one in contract/migration: slice the leading 13 characters, no check.
Two copies of a convention that applications also have to follow is two
things to keep in step, and the copies had already stopped matching - core
now rejects a name that carries no timestamp, and this one still accepted
"add_orders.go" and registered a migration under that string as its
version, which nothing would ever match and nothing would report.
Delegate instead, so there is one implementation of the rule and an app's
migration and a host migration derive their version the same way.
The test pins the reject case, not just the happy path: a re-divergence
that only sliced would still pass the happy path.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
core's seed package defines what an application may ask for and leaves the
writing to the host, which is the only side that knows its own tables. No
host implemented it, so SeedMenus returned ErrNoSeeder and an application's
menus never appeared.
adminSeeder writes all four kinds of row, not the two an obvious reading
would stop at: without sys_menu_api_rule and the sys_role_menu / casbin_rule
grants, the menu exists and no role can reach it.
Ids are always autoincrement, never caller-assigned - checksilent's
menu-id-collision check reads literals in this repository's tree and cannot
see an application in the module cache, so the collision is removed by
construction instead of guarded. The runtime validation covers what a static
scan cannot reach for a third-party spec: duplicate codes, unresolved parents
and api references, an unknown kind, and a sort outside sys_menu.sort's
tinyint range.
MenuSpec carries no menu name, so one is synthesised from the app code and
the spec code - two applications both choosing "list" would otherwise collide
on the frontend's keep-alive key.
It lives in app/admin/service because cmd links both subcommands into one
binary, so its init runs whichever one is invoked, and cmd/migrate never has
to import app/admin to reach it.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
sys_migration already carries app_code; sys_menu and sys_api did not, so
nothing said which application seeded a row - which is what an uninstall or
an audit would have to ask.
The migration adds the columns through the runtime models rather than
cmd/migrate/migration/models, whose frozen ModelTime is wrong for anything
ordered after the soft-delete conversion.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
core's sdk/contract/migration keeps its own process-wide registry, because
that is the only door open to an application that must not import the host.
Nothing here ever opened it: ForApp("crm").SetVersion(...) compiled,
registered, and then never ran - no error, no mention in status, nothing.
mergedEntries unions the host's own registry with contract/migration's
Snapshot(), and status, run and AppCodes all read through it, so migrate,
status, --dry-run and --app see an application's migrations exactly as they
see the host's. Version namespacing already keeps the two apart, so a key
collision should not be reachable; the host's own registration wins if one
ever is, rather than being silently replaced.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The comment said the test had to be declared first because Go runs a
package's tests in source order. That is not a guarantee, and it is not what
makes this work: the test that registers RoleCheck puts it back in a
t.Cleanup, and the guard here turns a wrong order into a loud failure rather
than a silent pass. Verified with go test -shuffle on seeds that run the two
in either order.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The Create handler's @Param named dto.OrderCreateReq, but this file imports
that package as orderdto. swag stops on it:
ParseComment error ... cannot find type definition: dto.OrderCreateReq
The @Success annotations name models.Response, which resolves - through
--parseDependency - to core's sdk/contract/models.Response rather than to
this package. That is the right envelope, and worth a note next to the
import, because the obvious "correction" is wrong: core's response.Response,
which the framework's own handlers name, carries no data field, so switching
to it would document these endpoints as returning no payload.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
sys_menu.sort is `gorm:"size:4"`, which MySQL builds as a tinyint holding
-128..127. Sort: 200 passes every sqlite test - sqlite ignores the width -
and fails on a real install with Error 1264, partway through a migration.
This is the exact incident class checksilent's menu-sort-overflow check
exists to prevent, and it reached a hand-written deliverable anyway: that
check only recognises a SysMenu literal from the host's model packages, so
a seed.MenuSpec is invisible to it. Widening the check is tracked
separately; this is the value it would have caught.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
Registration goes through core's package-level facades: SetAppRouters for
the routes, migration.ForApp for the schema, and seed.MenuSpec/ApiSpec
for the menu rows - none of which requires importing the host.
The menu component is spelled apps/order/order/index. The frontend tells
a packaged view from a built-in one by that first segment alone, and
getting it wrong is silent: the page falls back to the not-installed
placeholder while the console names a src/views path that was never going
to exist. The tests assert that prefix, that every Parent reference
closes, and that every ApiCode resolves - the three ways a menu graph is
wrong without anything saying so.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
No generic CRUD action anywhere: real business - a cross-table order
placement, a payment transition - is what the contract surface has to
carry, and the actions cover only the single-table case that a real
application outgrows immediately.
The transaction is Orm.Transaction(), not the Begin/defer shape that
app/admin/service/sys_role.go and three other files use. That shape
commits a half-written transaction when the body panics, because the
deferred check reads err, which a panic leaves nil.
The payment transition guards concurrency through the update itself -
WHERE status = 'pending' plus RowsAffected - rather than a read followed
by a write.
The tests cover both rollback paths, because they fail differently: a
mid-transaction error returns, a panic unwinds - and the second is what
tells Orm.Transaction() apart from the shape it replaces. The concurrency
test pins the pool to one writer so sqlite's own single-writer semantics
cannot stand in for the guard being tested. The data-scope tests assert
the fail-closed direction too: an unrecognised scope must return no rows
rather than every row.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
A reference application for a third-party author: its own module, and a
require list that names go-admin-core and nothing else. The point of the
example is that constraint - an application that reaches for the host
cannot be installed through a module proxy at all, because `go-admin` has
no dot in its first path element and a replace directive is ignored
outside the main module.
Two tables rather than one, because a single-table example proves only
what the generic CRUD actions already proved. The interesting question is
whether the contract surface holds up for business that spans tables.
The table names carry an app_ prefix: "order" is a reserved word, and
Permission() interpolates the table name into raw SQL without quoting.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
Two properties the previous shape broke silently: GetHandlerFunc must
report ok for the JwtToken key, and every module must read back the same
instance. Reverting the registration to the unbound method expression
still compiles and turns the first of these red, which is the failure
this pins.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
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
IndexAction closes over one dto.Index and serves every request to the route
from it; Generate exists so each request gets its own instance, and every
implementation in this repository returns a copy for that reason. The probe
returned the receiver, which made it the one shape IndexAction is not
written against - and inconsistent with probeRow in the same file, which
already copied.
A single-request test cannot tell the two apart, so the assertion is on
Generate itself rather than on the action's behaviour.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
create.go/delete.go/index.go/update.go/view.go were not lowered to
core (PRD 006 F3) and still call actions.Permission directly, in this
repository, on a code path core's own test suite knows nothing about:
core pins down what Permission builds for a given scope, but nothing
covered whether this package's five Actions still remember to call it
at all. TestIndexActionAppliesDataPermission runs IndexAction exactly
as a real request would, against a real in-memory database, and
inspects the SQL GORM actually executed - not just that the handler
returned success, which it would just as happily do with the filter
missing entirely.
The SQL is captured through a gorm.io/gorm/logger.Interface wrapper
rather than read back from IndexAction's own *gorm.DB: IndexAction
builds and executes its query in one unbroken chain
(Model().Scopes().Find()...Count()) and never hands the built
statement back to its caller, so there is nothing else to inspect it
through.
Counterproof performed and reverted (not part of this commit): with
Permission(object.TableName(), p) removed from IndexAction's Scopes
call, the test failed with the captured SQL carrying no WHERE clause
at all (`SELECT * FROM action_probe_row LIMIT 10`); index.go was then
restored to its committed content (`git diff --exit-code` verified
clean).
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
DataPermission, PermissionAction, Permission, GetPermissionFromContext,
IsValidDataScope, PermissionKey and the five DataScope* constants now
forward to go-admin-core's sdk/contract/actions, which carries the
already-fixed logic from feat/006-security-prereq (PRD 006 F14/H1-H3).
create.go/delete.go/index.go/update.go/view.go - the five generic CRUD
actions - are untouched: they call Permission and
GetPermissionFromContext by the same names, which now resolve to
forwards with identical behaviour, and stay in this package rather
than moving to core (PRD 006 F3: core's exports are a permanent
promise every fork inherits, and CRUD shape is this framework's most
volatile surface).
PermissionKey is declared as `const PermissionKey =
contractactions.PermissionKey`, a direct reference rather than a
restated literal, per PRD 006's hard constraint 4: PermissionAction
sets this gin context key and GetPermissionFromContext reads it back,
and an independently declared copy could silently drift from core's if
one were ever edited without the other. permission_test.go replaces
the detailed data-permission regression suite - which now lives in
core, next to the logic itself - with a test of this package's own
wiring: that PermissionAction and both of this package's own read
paths (GetPermissionFromContext, and c.Get(actions.PermissionKey)
directly) still meet on the same key.
Counterproof performed and reverted (not part of this commit): with
PermissionKey redeclared here as the literal "dataPermission" and
core's copy changed to a different value, TestPermissionKeyMatches-
WhatPermissionActionSets went red while GetPermissionFromContext's own
round-trip stayed green - confirming the exported constant, not the
GetPermissionFromContext wrapper, is what an independent literal would
put at risk.
PRD 006 F3/F5.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
AutoForm, ObjectById/ObjectGetReq/ObjectDeleteReq, Pagination,
GeneralDelDto/GeneralGetDto and Index/Control are now type aliases of
go-admin-core's sdk/contract/dto; OrderDest, MakeCondition and
Paginate forward to the same package (functions cannot be aliased the
way types can).
MakeCondition no longer reads common/global.Driver to choose which SQL
dialect to resolve search tags against. The lowered version reads
db.Dialector.Name() from inside the closure it returns instead, which
is always the driver the caller's own *gorm.DB is bound to - correct
even with more than one database open with different drivers, which a
single process-wide variable could never be. global.Driver is marked
Deprecated accordingly; it is still set and still readable for fork
code that reads it directly.
PRD 006 F2/F5.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
ControlBy, Model, ModelTime, ActiveRecord, BaseUser, Response, Page,
Migration and the menu type constants now read `type X = pkg.X` /
`const X = pkg.X` against go-admin-core's sdk/contract/models instead
of defining these shapes locally. Every embed, GORM tag and JSON tag
is unchanged - a type alias is the same type, not a new one - and
every existing import of go-admin/common/models keeps compiling with
no changes of its own (verified with `git diff --exit-code` over the
70 files that import common/models, common/dto or common/actions).
The menu type constants (Directory/Menu/Button) are declared as direct
references rather than restated literals: an independently written
copy of the same value can be edited out of step with go-admin-core's,
where a direct reference cannot (PRD 006 hard constraint 4).
PRD 006 F1/F5.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
zhangwenjian