[jwrosewell]

The problem

The JSON Builder Element needs a sequence number, and this repository had no
test at all for what happens when there is none. The element did fail, and it
has done so since long before this release, but the message a caller read named
neither the element nor what to do about it, and it was not the message any
other language gave. The release is about every language behaving the same way
and saying the same thing, so the message moves onto the one the specification
now gives and eight tests hold the whole behaviour in place.

What the specification says

json-builder.md at d72eb074, lines 120 to 142,
which is the commit where
specifications#139
merged:

If no Sequence Element has produced a sequence number when the Element runs,
the Element MUST fail with an error and MUST NOT set its JSON value. The error
MUST carry the message below, which is the text every implementation gives, so
that the message reads the same in every language and makes life easier for
51Degrees support.

Error	Message
No Sequence Element has produced a sequence number	The JSON Builder needs a sequence number. Place a Sequence Element before the JSON Builder in the Pipeline.

[...] Where an implementation wraps the error in another, such as a Pipeline
Configuration Exception that carries the first error as its cause, the error
the caller reads carries the message.

The message is the whole text of the error. An implementation MUST NOT add
anything to it, such as a link to documentation. A prefix that a language's
error type writes in front of every error of that type when the error is
shown, such as the type's name, is not part of the message.

Lines 150 to 153
add that a query.sequence value taken from a query string is text and not an
integer, so it does not count as a sequence number, and that an integer a
caller added to query.sequence without a Sequence Element does count in Java,
because the Element reads the integer in Evidence whoever added it.

What changed

Three files, two of them production and one new test class.

1. Constants.java lines 28 to 39
   gains NO_SEQUENCE_NUMBER_MESSAGE, holding the page's sentence once so that
   the two throws and the tests cannot drift apart, and so another class can
   reach the same words. Its Javadoc links the page at the commit the words
   came from.
2. JsonBuilderElement.java lines 206 to 209.
   buildJson caught the failure and threw a PipelineConfigurationException
   reading "Make sure there is a SequenceElement placed before this
   JsonBuilderElement in the pipeline". That is the error a caller reads, so
   under the page's wrapping sentence it now carries the specification's
   message and nothing else.
3. JsonBuilderElement.java line 229.
   getSequenceNumber threw "Sequence number not present in evidence. this is
   mandatory.", which becomes the cause and is the line at the top of a stack
   trace, so it carries the same sentence now.
4. JsonBuilderSequenceTests.java,
   a new class of eight tests, described below.

Nothing else about the element moved. It already failed without a sequence
number, already set no JSON when it failed, because
line 165
calls setJson only after buildJson has returned, already took an integer
from Evidence whoever put it there, and already refused query string text,
because
FlowDataDefault.tryGetEvidence lines 140 to 150
casts with type.cast(obj) and catches the ClassCastException. The tests pin
all of that, so none of it can be lost quietly later.

The eight tests

Test	What it holds
JsonBuilder_TheMessage_IsTheOneTheSpecificationGives	The constant's text, written out again in the test, so changing the constant fails here rather than carrying the other seven along with it
JsonBuilder_NoSequenceElement_Fails	No sequence element and no Evidence gives one error with that message and no JSON
JsonBuilder_SequenceFromQueryString_Fails	A query.sequence of "4" is text, so it fails the same way
JsonBuilder_SequenceThatIsNotANumber_Fails	A query.sequence of "abc" fails the same way
JsonBuilder_NoSequenceElement_ThrowsWhenNotSuppressed	With the pipeline not suppressing exceptions the failure reaches the caller inside an AggregateException, carrying the same message, and still no JSON
JsonBuilder_NoSequenceElement_TheCauseSaysWhatIsNeeded	The cause carries the same message as the error the caller reads
JsonBuilder_IntegerFromTheCaller_BuildsTheJson	An integer 3 added by the caller is a sequence number even with no sequence element, so the JSON is built and no error is recorded
JsonBuilder_WithSequenceElement_BuildsTheJson	A sequence element in front builds the JSON, which is the order the web integration assembles

The four failure tests go through one helper, which asserts three things each
time, being that exactly one error was recorded, that its message equals the
constant word for word, and that getJson() was never set.

Tests first, red then green

Three red runs on a runner, each one failing only on the thing the next commit
then fixed.

1. Run 42996855432
   on dfb2a64e, which takes the element's sequence number check out so the
   new tests have something to fail against.
   Tests run: 6, Failures: 4 in JsonBuilderSequenceTests, the four being
   JsonBuilder_NoSequenceElement_Fails,
   JsonBuilder_SequenceFromQueryString_Fails,
   JsonBuilder_SequenceThatIsNotANumber_Fails and
   JsonBuilder_NoSequenceElement_ThrowsWhenNotSuppressed. 6563209c puts the
   check back and
   run 42998105846
   is green on it.
2. Run 43016069363
   on 68b399bf, which adds the test for what the cause says.
   Tests run: 7, Failures: 1, reading "Expected the cause to say that a
   Sequence Element needs to be placed before the JSON Builder Element in the
   Pipeline. Complete message: Sequence number not present in evidence. this is
   mandatory."
3. Run 43019377909
   on 38900cbc, which moves every assertion onto the specification's exact
   sentence before the element gives it. Tests run: 7, Failures: 5, the five
   being every test that reads a message. 66bedcbd then puts the sentence
   into both throws and
   run 43020802655
   is green on it.

The full build

Run 43034325507,
completed/success, on this draft's head fd7573c7. Every
Setting 'PullRequestSha' line in the Configure job and in all nine Build and
Test jobs reads
fd7573c7b4e53512d864f3c405c515440305f571, which is the head. Configure
printed Left out 6 macOS entries. Eleven jobs succeeded and the two
performance comparison jobs were skipped, with none failing.
JsonBuilderSequenceTests reads Tests run: 8, Failures: 0, Errors: 0, Skipped: 0 on every one of the nine platform and Java version combinations.

macOS was left out of this draft's full build under the 9 November decision.
The rc head is built with macOS before it merges into main.

Why that build still stands, although rc has moved since

The build ran at 14:00 and rc has taken eight commits since, so it is fair to
ask whether the result still describes the merged tree. The draft is still
MERGEABLE/CLEAN, being eight commits behind and none in front, and I read the
eight rather than assuming they were harmless.

• Nothing on rc asserts the old text. The two throws used to read "Make
  sure there is a SequenceElement placed before this JsonBuilderElement in the
  pipeline" and "Sequence number not present in evidence. this is mandatory.".
  Neither string appears anywhere in JsonBuilderTests.java,
  FiftyOneInterceptorTests.java, FiftyOneJSServiceCoreTests.java or
  PipelineFilterTests.java at bf66e3d1, which are the four test classes the
  eight commits touched that could have held it.
• The Sequence Element still adds the Evidence these tests need.
  SequenceElement.java line 106 at bf66e3d1
  still calls data.addEvidence(Constants.EVIDENCE_SEQUENCE, sequence),
  whatever else
  #80 now puts in Element
  Data, and SequenceElementBuilder keeps the constructor and build() the new
  test class calls.
• The other open sequence draft does not collide either.
  51degrees-java#102
  changes SequenceElement.java, JavaScriptBuilderElement.java and the
  JavaScript Builder's tests. The lines it takes out are the JavaScript
  Builder's own tryGetEvidence calls, not the Sequence Element's
  addEvidence, so the route the JSON Builder reads is untouched.
• No submodule moved in those eight commits, so the checkout failure that
  catches a draft holding an older native library does not apply.

I have not merged rc in, because moving the head would mean another full build
and a rewritten description for no change in what the draft does.

What remains

• Only the patch number moves. No version directive is in any commit message or
  anywhere in this description.
• 51degrees-java#106,
  which replaced the same two throws without the tests, is closed, so there is
  no longer a second draft of this work to reconcile.
• The same sentence is going into the other languages in their own drafts, and
  nothing in this one waits on them.
  51degrees-rust#383 and
  51degrees-go#100 have
  merged, and
  51degrees-dotnet#135
  is still open at the time of writing.
• The constant and the test each write the words out in full and separately, so
  if the page is ever reworded this repository is two lines and one build.