fix(timerange): honour bound markers and return client time strings as sent - #24
Open
Akshay-Narayan-Pai wants to merge 21 commits into
Open
Akshay-Narayan-Pai wants to merge 21 commits into
Akshay-Narayan-Pai wants to merge 21 commits into
Conversation
Registering an instantaneous segment at time 0 ([0:0]) returns 500, because metastore.nsRange ignores the bound markers: [0:0] becomes the empty range (0, 0) and fails the segments_upper_ns_positive check. The same defect drops the last nanosecond of every closed range, so some overlapping segments are accepted. TAMS says "Service implementations MUST take clusivity markers of the timerange into account." Tracing the defect showed that several design rules were missing or wrong, so this change corrects the specification before the code: - ADR-0038: parse timestamps and timeranges by the TAMS grammar (App Note 0008), and follow BBC's mediatimestamp library where the note is silent. The sign applies to the whole value, so -1:500000000 is -1.5 s. One-sided ranges, bare timestamps, and empty ranges parse. - ADR-0039 (supersedes ADR-0014 on acceptance): store each client time string exactly as sent, derive half-open nanosecond bounds only in timerange.NsBounds with checked arithmetic, use the bounds only for comparison, and name one predicate per query. DELETE uses containment, as TAMS requires. - ADR-0040 (supersedes ADR-0015 on acceptance): a segment timerange must be bounded, non-empty, and start-inclusive, reported as a per-segment failure. upper_ns becomes NOT NULL and the check becomes upper_ns > lower_ns. Migration 000005 is edited in place, and v0.1.0-alpha.0 databases are fixed by hand. The design documents for internal-timerange, internal-metastore, internal-service-segment, internal-service-flow, and internal-httpx-conversion follow the ADRs. Every changed rule is marked "(pending)" until the implementation lands. conformance.md records the segment timerange policy and the DELETE defect, and replaces a "Time-range syntax" section that described behaviour the parser does not have.
Akshay-Narayan-Pai
changed the title
Akshay-Narayan-Pai
changed the title
Akshay-Narayan-Pai
changed the title
Rewrites the text added in the previous commit to the simple-English (ASD-STE100, pragmatic) rules: - Active voice where the agent is known: the parser, the store, the service, or the handler. - No "would", "now", "therefore", "reach", or "perform" outside quoted specification text. - No semicolons, and no "below" or "above" as references. Links name the target section. - Sentences within 25 words, lists with a lead-in colon, and no nested list. Also fixes a structural error from the previous commit: the BR-SEG-02 insertion split the existing paragraph, so the sentence about client-supplied GetURLs had moved below the new timerange rules. It is back in its original paragraph. Verbatim TAMS quotes are unchanged. British spelling stays, to match the rest of the repository.
TAMS never calls the timestamp and timerange string definitions a grammar. The schemas and the API document say "format", and App Note 0008 titles its section "TimeRange representation". Renames ADR-0038 to 0038-tams-timestamp-and-timerange-format.md and updates its title, the ADR index, every link to it, BR-TR-06, BR-TR-13, and the conformance.md section heading, which becomes "Time-range format". No document linked to the old heading anchor.
- BR-META-21: give explicit SQL for all three ListFlows cases: no filter for "_", NOT EXISTS for an empty range, and EXISTS with overlap for a non-empty range. Record that OpenTAMS reads "Flows that overlap" as "a segment of the flow overlaps", and that HEAD /flows follows GET, although the upstream HEAD text omits the empty-range sentence. - BR-META-10 and the metastore domain types: state how the raw client strings travel through domain.Segment.*Raw from conversion to the store and back. Remove the stale Segment struct copy. FailedSegment and SegmentPage point at domain.Segment. - BR-TR-07: define the Overlaps/NsBounds property for every pair of ranges, including empty and unbounded ones. - BR-TR-10: give the exact CI command and its scope (server code under internal/, pkg/, cmd/). Use git grep -w, because git grep -E does not support \b. State the exact int64 nanosecond limits. - BR-TR-12: write "-" only for a negative value, so String() never emits "-0:0". - BR-TR-14 (new): TimestampFromDuration replaces durationToTAI in handlers/root.go, because TAMS types min_object_timeout as a Timestamp. - ADR-0038 rule 9 (new) and BR-TR-05, BR-CONV-09: the schema regex accepts "(10:0)", but the schema text forbids exclusive markers on an instantaneous range. OpenTAMS follows the text, not mediatimestamp. - conformance.md: exact representable range, the TAMS source of the empty-timerange rule for GET /flows, the ListFlows interpretation, and HEAD /flows.
The block in the metastore business rules had drifted from the code: - ListSegmentsParams does not exist. ListSegments takes ListQuery. - ListSourcesParams was missing Label, Format, TagExists, and TagValues. - ListFlowsParams was missing FrameWidth and FrameHeight, and named the type timerange.Timerange instead of timerange.TimeRange. - FailedSegment and SegmentPage belong to internal/domain, and both copies had wrong or missing fields. - InsertBatch, InsertResult, RejectReason, ListQuery, DeleteQuery, and DeleteResult were missing. The block now copies only the types that internal/metastore owns, each checked against flows.go and segments.go, and links to internal/domain for the segment types. Source, Flow, CollectionItem, SourcePage, and FlowPage already matched and are unchanged.
Lower is inclusive and Upper is exclusive. The field comments now say so, and the type comment names the half-open interval, so a reader does not take Upper as the last instant.
Implement ADR-0038 in internal/timerange: - The sign applies to the whole timestamp value, stored floor-normalised. -1:500000000 is -1.5 s, and -0:x is valid. - One-sided ranges, bare timestamps, omitted markers, and the other regex-valid forms parse as mediatimestamp parses them. - End before start parses as an empty range, not as an error. - An instant is a whole nanosecond (new ADR-0038 rule 10), so (0:0_0:1) is empty, and Overlaps is defined through Intersect. - String renders [ts] for an instant, () for every empty range, and never writes -0:0. - NsBounds converts a range to half-open int64 bounds with checked arithmetic, ErrOutOfRange, and ErrEmptyRange. - TimestampFromDuration replaces ad hoc duration arithmetic. Docs: BR-TR-04 to BR-TR-07 and conformance record the regex-only forms and the whole-nanosecond rule.
Add TimerangeRaw, TSOffsetRaw, ObjectTimerangeRaw, and LastDurationRaw, so conversion, service, and metastore can return the exact strings a client sent (BR-CONV-08, BR-META-10).
A segment timerange must be bounded, non-empty, start-inclusive, and fit in int64 nanoseconds (ADR-0040 rules 1-2, BR-SEG-02). A segment that breaks a rule is an invalid-timerange failure in Failed and never reaches the store; the other segments in the batch continue. A List or Delete query timerange outside the int64 nanosecond range is invalid-timerange (BR-SEG-09); an empty query range goes to the store. The failure log carries the client's timerange string.
SegmentFromAPI and the POST decoder keep the exact wire string of timerange, ts_offset, object_timerange, and last_duration in the domain *Raw fields next to each parsed value. SegmentToAPI and RegisterFailureToAPI write the raw strings and never String() of a parsed value, so a client that sends [10:0] or -0:0 reads back the same bytes (BR-CONV-08). A string that does not parse, such as (10:0), stays a request-level schema-validation error; an empty range parses and passes through to the service (BR-CONV-09).
Remove durationToTAI, which did its own nanosecond arithmetic and rendered a negative duration as -1:-500000000. GetService now uses timerange.TimestampFromDuration(d).String() (BR-TR-14).
…merange
GET and HEAD /flows returned every flow when the timerange did not parse;
they now return 400 invalid-timerange. HEAD /flows uses the GET parameter
parser, so both apply the same filter, including an empty range
(ADR-0039 rule 7, BR-CONV-09).
GET, HEAD, and DELETE /flows/{flowId}/segments map the service's
invalid-timerange error for an out-of-range query to 400 (BR-SEG-09).
A store error the handler does not map, such as a constraint violation,
stays a 500 with the idempotency key released (ADR-0040 rule 3,
BR-IDMP-02); tests now pin that a retry with the same key is processed.
Add the BR-TR-10 step: it fails if git grep finds 1_000_000_000, 1000000000, or 1e9 in non-test server code outside internal/timerange. git grep status 1 (no match) passes; a status above 1 fails as an error.
Replace segments_upper_ns_positive, which rejected valid negative ends and accepted empty ranges, with upper_ns NOT NULL and segments_bounds_nonempty CHECK (upper_ns > lower_ns) (ADR-0040 rule 4). Edited in place; the down migration reverts both.
Remove nsRange, which ignored bound markers, so [0:0] became (0, 0) and failed the CHECK constraint with a 500. Every conversion now goes through timerange.NsBounds (ADR-0039, BR-META-10, BR-META-21): - insert and both overlap checks compare one set of half-open bounds - an empty or out-of-range segment timerange fails the batch - the four time columns hold the client strings, read back as *Raw - ListSegments uses overlap, DELETE uses containment - ListFlows matches a flow when one of its segments overlaps; an empty query returns flows with no segments; _ applies no condition - the flow and page timeranges are joined from parsed stored strings and rendered with TimeRange.String(), not built from nanoseconds
BR-META-21: the store returns timerange.ErrOutOfRange for a query timerange that does not fit in int64 nanoseconds, and the caller maps it to 400 invalid-timerange. GET and HEAD /flows returned 500. Also rename the loader's allowed-survivor entry to the new segments_bounds_nonempty constraint.
The handler ignored a timerange that did not parse and returned the flow unfiltered. It now returns 400 invalid-timerange, like every other timerange query (BR-CONV-09, SCN-HTTP-08). The spec declares no 400 for this endpoint, so the handler returns an AppError and the error middleware writes the problem.
InsertSegments rejects a segment without TimerangeRaw (BR-META-10). The
concurrency integration tests and the scaletest bench now set it.
Docs: BR-META-10 records how absent optional strings are stored and the
TimerangeRaw requirement. BR-CONV-09 and conformance record 400
invalid-timerange for every timerange query parameter, including GET
/flows/{flowId}, which the upstream spec declares no 400 for.
The code for the three ADRs has landed. Set them to accepted, set ADR-0014 and ADR-0015 to superseded, remove the (pending) markers, and remove the notes that described the defects this branch fixes.
Akshay-Narayan-Pai
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // such as a database constraint violation, which the service's | ||
| // validation makes unreachable from client input (ADR-0040 rule 3). | ||
| // A 5xx is never cached; the key is released (BR-IDMP-02). | ||
| log.Error("RegisterBatch failed; releasing idempotency key", zap.Error(err)) |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #25
Why
Registering an instantaneous segment at time 0 (
[0:0]) returns 500:metastore.nsRangeignored the bound markers.[0:0]became the empty range(0, 0), and every closed range lost its last nanosecond, so the store accepted some overlapping segments. TAMS says: "Service implementations MUST take inclusivity markers of the timerange into account."Tracing the defect showed that the design rules themselves were missing or wrong. This PR corrects the specification first (ADRs and design docs), then implements it test-first.
Decisions
mediatimestampwhere the note is silent, and the spec text where the two disagree. The sign applies to the whole value (-1:500000000= −1.5 s). One-sided ranges, bare timestamps, and empty ranges parse. An instant is a whole nanosecond, so(0:0_0:1)is empty (rule 10).timerange.NsBoundsis the only conversion to half-open nanosecond bounds, with checked arithmetic. Bounds are for comparison only. One named predicate per query: overlap for GET/HEAD, containment for DELETE. ListFlows matches a flow when one of its segments overlaps.invalid-timerangefailure in a 200.upper_ns NOT NULL,CHECK (upper_ns > lower_ns). A constraint violation is a server defect: 500, idempotency key released.What changed
internal/timerangeNsBounds()withErrOutOfRangeandErrEmptyRange.TimestampFromDuration.String()is canonical and is used only for derived values. It renders[ts]for an instant and never writes-0:0.Metastore and migration
000005is edited in place:upper_nsisNOT NULL, andsegments_bounds_nonemptyreplacessegments_upper_ns_positive.nsRangeis deleted. Insert, overlap, list, delete, and ListFlows all useNsBounds.domain.Segment.*Raw). These are internal fields, and the API does not change.X-Paging-Timerangeare built from the parsed stored strings.Service, conversion, and handlers
invalid-timerangefailures.int64, returns 400invalid-timerange. This covers GET, HEAD, and DELETE segments, GET and HEAD/flows, and GET/flows/{flowId}./flowsshares the GET filter.durationToTAIis replaced byTimestampFromDuration.CI
1_000_000_000,1000000000,1e9) in server code outsideinternal/timerange.Docs
internal-timerange,internal-metastore,internal-service-segment,internal-service-flow, andinternal-httpx-conversion.conformance.md.Tests
Every edge case from the analysis has a test. Each test was seen failing before its fix. Highlights:
[0:0]end to end.[1:0_2:0)then[2:0]._.A property test checks 50,000 random pairs and shows that
Overlapsalways agrees with the bounds comparison. It found the whole-nanosecond case that became ADR-0038 rule 10.Verified locally:
go build,go vet, andgolangci-lint(0 issues).go test -short -race ./....-tags=integration -race, testcontainers).Upgrading a
v0.1.0-alpha.0databasegolang-migratedoes not re-run the edited000005. Follow ADR-0040 rule 7 by hand:Rows written before this PR hold
'0:0'for an absentts_offset, and now read back as an explicit"0:0".Out of scope (follow-ups)
ts_offsetas0:0, as the spec says. No logic reads the value today.