Reducing cache-control for scheduled transactions. #9327

mgoelswirlds · 2024-09-11T01:28:25Z

Description:
Reduce cache for the transactions endpoint with scheduled transactions.

This PR modifies

Reduces the value of max-age for ScheduleCreate transactions
Changes the way content-type is set for the /network/supply endpoint

Related issue(s):

Fixes #9174

Notes for reviewer:
Checklist

Documented (Code comments, README, etc.)
Tested (unit, integration, etc.)

Signed-off-by: mgoelswirlds <[email protected]>

hedera-mirror-rest/__tests__/specs/transactions/{id}/no-params.json

…edule-transactions

codecov · 2024-09-11T14:39:59Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.68%. Comparing base (90dca10) to head (45ab8c3).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #9327   +/-   ##
=========================================
  Coverage     92.68%   92.68%           
  Complexity     6943     6943           
=========================================
  Files           896      896           
  Lines         29553    29576   +23     
  Branches       3745     3751    +6     
=========================================
+ Hits          27390    27412   +22     
  Misses         1399     1399           
- Partials        764      765    +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: mgoelswirlds <[email protected]>

hedera-mirror-rest/middleware/responseHandler.js

hedera-mirror-rest/transactions.js

...ra-mirror-rest/__tests__/specs/transactions/{id}/scheduled-transactions/scheduled-false.json

hedera-mirror-rest/middleware/responseHandler.js

hedera-mirror-rest/transactions.js

Signed-off-by: mgoelswirlds <[email protected]>

steven-sheehy · 2024-09-15T15:14:20Z

hedera-mirror-rest/middleware/responseHandler.js

+import {
+  requestPathLabel,
+  requestStartTime,
+  responseContentType,


We should update network supply to use the new response headers and remove responseContentType to make it consistent and not have two ways to possibly set content type.

I did try changing this. Works for network supply but many other tests fail complaining about the Invalid media type. Looking further into what needs to be resolved here.

hedera-mirror-rest/transactions.js

mgoelswirlds · 2024-09-16T13:17:56Z

I still need to address the new comments on the PR @steven-sheehy
I mistakenly asked for a re-review.

hedera-mirror-rest/transactions.js

...irror-rest/__tests__/specs/transactions/{id}/scheduled-create-non-scheduled-transaction.json

…edule-transactions

Signed-off-by: mgoelswirlds <[email protected]>

hedera-mirror-rest/transactions.js

Removing the use of responseHeadersLabel. Signed-off-by: mgoelswirlds <[email protected]>

Signed-off-by: mgoelswirlds <[email protected]>

hedera-mirror-rest/middleware/responseHandler.js

hedera-mirror-rest/transactions.js

Signed-off-by: mgoelswirlds <[email protected]>

mgoelswirlds · 2024-09-17T23:49:58Z

hedera-mirror-rest/__tests__/middleware/responseHandler.test.js

-    expect(mockResponse.set).toHaveBeenNthCalledWith(1, headers.default);
-    expect(mockResponse.set).toHaveBeenNthCalledWith(2, headers.path[mockRequest.route.path]);
-    expect(mockResponse.set).toHaveBeenNthCalledWith(3, 'Content-Type', 'application/json; charset=utf-8');
+    expect(mockResponse.set).toHaveBeenCalledWith({'Content-type': 'application/json; charset=utf-8'},headers.path[mockRequest.route.path]);


I have spent a lot of time trying to fix these 2 tests with this new approach. But have not been successful.
Would like to get the other parta approved and continue working on these on the side.

steven-sheehy · 2024-09-18T20:50:15Z

hedera-mirror-rest/model/transactionResult.js

@@ -363,8 +363,13 @@ const getSuccessProtoIds = () => {
  ];
 };

+const isSuccessful = (result) => {
+  return getSuccessProtoIds().includes(result);


For performance, we should calculate getSuccessProtoIds() once and store as a static. Prior to this method, its other uses were separately cached so it wasn't a concern. Now it's called in a loop inside our highest used API.

I have added a constant which calls getSuccessProtoIds() once.

steven-sheehy · 2024-09-18T20:51:20Z

hedera-mirror-rest/middleware/responseHandler.js


    const code = res.locals.statusCode;
-    const contentType = res.locals[responseContentType] || APPLICATION_JSON;
+    const contentType = mergedHeaders['Content-type'];


Use CONTENT_TYPE_HEADER here and other places that put responseHeadersLabel

steven-sheehy · 2024-09-18T20:55:52Z

hedera-mirror-rest/middleware/responseHandler.js

+    const contentType = mergedHeaders['Content-type'];
+
    const linksNext = res.locals.responseData.links?.next;
    res.status(code);
    res.set(CONTENT_TYPE_HEADER, contentType);


We shouldn't need to set content-type header since it should've already been set above via res.set(mergedHeaders).

Suggested change

const contentType = mergedHeaders['Content-type'];

const linksNext = res.locals.responseData.links?.next;

res.status(code);

res.set(CONTENT_TYPE_HEADER, contentType);

const linksNext = res.locals.responseData.links?.next;

res.status(code);

steven-sheehy · 2024-09-18T20:58:15Z

hedera-mirror-rest/middleware/responseHandler.js

+    const mergedHeaders = {
+      ...headers.default,
+      ...(headers.path[path] ?? {}),
+      ...(res.locals[responseHeadersLabel] ?? {}),
+    };
+    res.set(mergedHeaders);


Have to be careful since HTTP headers are case insensitive but this mergedHeaders object is not. However, express set() should be case insensitive. We should have a unit test that has different mixed case for default, path and responseHeadersLabel and ensure they are all merged into one entry returned via res.get()

I tried adding a unit test, but that would need to work with mocking the result and then there seemed no point to hardcode what set and get would do.
This needs fixing but this was the tentative test:

test('should merge headers in a case insesitive manner', async () => { const path = '/api/v1/transactions/:transactionId'; headers.path[path] = { "Cache-control": "public, max-age=1", "content-Type": "application/json; charset=utf-8" }; mockResponse.locals[responseHeadersLabel] = { "cache-Control": "public, max-age=1", "Content-type": 'text/plain; charset=utf-8' }; await responseHandler(mockRequest, mockResponse, null); expect(mockResponse.get('content-type')).toHaveLastReturnedWith(mockResponse.locals[responseHeadersLabel]['Content-type']); });

The network/supply spec tests do test the case insensitiveness to some extend atleast for headers.default and res.locals[responseHeadersLabel]
If that is acceptable , I can work on a future PR to add a more comprehensive test for this .

steven-sheehy · 2024-09-18T21:01:10Z

hedera-mirror-rest/transactions.js

+function getTransactionsByIdOrHashCacheControlHeader(transactionsRows, isTransactionHash, scheduled) {
+  if (scheduled || isTransactionHash) {


As I mentioned previously.

Suggested change

function getTransactionsByIdOrHashCacheControlHeader(transactionsRows, isTransactionHash, scheduled) {

if (scheduled || isTransactionHash) {

function getTransactionsByIdOrHashCacheControlHeader(transactionsRows) {

if (transactionsRows.length < 2) {

not entirely true. it's possible to have scheduled=false query param and return multiple transactions for the same transaction id. Then we can have a successful schedulecreate in the response with all others failed, ideally we should use the longer max-age because the existence of the scheduled=? query param.

However given that such scenario happens rarely, and using a shorter max-age doesn't hurt function, agree we can go with the simpler approach.

I missed that this scheduled variable represents the query param exists and not that it exists and is set to true, hence my comment. We can keep the scheduled check but omit isTransactionHash and just use list size for that.

steven-sheehy · 2024-09-18T21:05:40Z

hedera-mirror-rest/config/application.yml

@@ -87,7 +87,8 @@ hedera:
      response:
        compression: true
        headers:
-          default: { "cache-control": "public, max-age=1" }
+          default: { "cache-control": "public, max-age=1",
+          "Content-type": "application/json; charset=utf-8"}


Use consistent casing.

Suggested change

"Content-type": "application/json; charset=utf-8"}

"content-type": "application/json; charset=utf-8"}

Actually other places in the code so you "Content-type" but will change it here.

steven-sheehy · 2024-09-18T21:37:38Z

hedera-mirror-rest/transactions.js

+    if (transaction.type === scheduleCreateProtoId) {
+      if (TransactionResult.isSuccessful(transaction.result)) {


We can combine these two if statements since it's not like a schedule create can have scheduled=true

steven-sheehy · 2024-09-18T21:39:53Z

hedera-mirror-rest/transactions.js

+  if (successScheduleCreateTimestamp !== undefined
+      && (utils.nowInNs() - successScheduleCreateTimestamp) < maxTransactionConsensusTimestampRangeNs ) {


nit: Readability

Suggested change

if (successScheduleCreateTimestamp !== undefined

&& (utils.nowInNs() - successScheduleCreateTimestamp) < maxTransactionConsensusTimestampRangeNs ) {

const elapsed = utils.nowInNs() - successScheduleCreateTimestamp;

if (successScheduleCreateTimestamp && elapsed < maxTransactionConsensusTimestampRangeNs ) {

steven-sheehy · 2024-09-18T21:41:18Z

hedera-mirror-rest/transactions.js

+  const isTransactionHash = isValidTransactionHash(transactionIdOrHash);
+  let scheduledParamExists = false;


Think updates to this method can be undone with my other comment.

Signed-off-by: mgoelswirlds <[email protected]>

…edule-transactions

hedera-mirror-rest/transactions.js

Signed-off-by: mgoelswirlds <[email protected]>

hedera-mirror-rest/transactions.js

Signed-off-by: mgoelswirlds <[email protected]>

hedera-mirror-rest/controllers/networkController.js

Signed-off-by: mgoelswirlds <[email protected]>

sonarcloud · 2024-09-21T01:36:31Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

xin-hedera · 2024-09-21T02:01:41Z

hedera-mirror-rest/middleware/responseHandler.js

 import {NotFoundError} from '../errors';
 import {JSONStringify} from '../utils';

 const {
  response: {headers},
 } = config;

-const CONTENT_TYPE_HEADER = 'Content-Type';
+const CONTENT_TYPE_HEADER = 'content-type';


why have the same constant defined in two places?

xin-hedera · 2024-09-21T02:04:31Z

I approved it since I'm off on Monday so don't unnecessarily block the PR. There is still one minor change I asked for.

mgoelswirlds added 2 commits September 10, 2024 21:17

Reducing cache-control for scheduled transactions.

b25938d

Signed-off-by: mgoelswirlds <[email protected]>

Missed files.

8700740

Signed-off-by: mgoelswirlds <[email protected]>

mgoelswirlds commented Sep 11, 2024

View reviewed changes

hedera-mirror-rest/__tests__/specs/transactions/{id}/no-params.json Outdated Show resolved Hide resolved

Merge remote-tracking branch 'origin/main' into 9174-reduce-cache-sch…

26aecf7

…edule-transactions

mgoelswirlds self-assigned this Sep 11, 2024

mgoelswirlds added enhancement Type: New feature rest Area: REST API labels Sep 11, 2024

mgoelswirlds added this to the 0.114.0 milestone Sep 11, 2024

Merge remote-tracking branch 'origin/main' into 9174-reduce-cache-sch…

5f4fb3b

…edule-transactions

Fixing middleware test.

81d263e

Signed-off-by: mgoelswirlds <[email protected]>

mgoelswirlds marked this pull request as ready for review September 11, 2024 15:02

steven-sheehy requested changes Sep 11, 2024

View reviewed changes

xin-hedera requested changes Sep 11, 2024

View reviewed changes

hedera-mirror-rest/middleware/responseHandler.js Show resolved Hide resolved

hedera-mirror-rest/transactions.js Outdated Show resolved Hide resolved

mgoelswirlds added 2 commits September 11, 2024 23:34

Addressing PR comments.

7c388a9

Signed-off-by: mgoelswirlds <[email protected]>

Adding corner cases and tests.

2e0840e

Signed-off-by: mgoelswirlds <[email protected]>

steven-sheehy requested changes Sep 15, 2024

View reviewed changes

mgoelswirlds requested review from xin-hedera and steven-sheehy September 16, 2024 13:14

xin-hedera reviewed Sep 16, 2024

View reviewed changes

hedera-mirror-rest/transactions.js Outdated Show resolved Hide resolved

hedera-mirror-rest/transactions.js Outdated Show resolved Hide resolved

hedera-mirror-rest/transactions.js Outdated Show resolved Hide resolved

xin-hedera reviewed Sep 16, 2024

View reviewed changes

...irror-rest/__tests__/specs/transactions/{id}/scheduled-create-non-scheduled-transaction.json Outdated Show resolved Hide resolved

mgoelswirlds added 2 commits September 16, 2024 12:49

Merge remote-tracking branch 'origin/main' into 9174-reduce-cache-sch…

7aece37

…edule-transactions

Addressing PR comments.

4e737c4

Signed-off-by: mgoelswirlds <[email protected]>

xin-hedera requested changes Sep 16, 2024

View reviewed changes

hedera-mirror-rest/transactions.js Outdated Show resolved Hide resolved

mgoelswirlds added 5 commits September 16, 2024 21:59

Addressing PR comments.

26d48ec

Removing the use of responseHeadersLabel. Signed-off-by: mgoelswirlds <[email protected]>

Addressing PR comments.

b97bbe7

Signed-off-by: mgoelswirlds <[email protected]>

Addressing PR comments.

03e720a

Signed-off-by: mgoelswirlds <[email protected]>

Changing network supply to use response headers.

d2d727a

Signed-off-by: mgoelswirlds <[email protected]>

fix test

1a60a79

Signed-off-by: mgoelswirlds <[email protected]>

fix test

6736a2c

Signed-off-by: mgoelswirlds <[email protected]>

xin-hedera reviewed Sep 17, 2024

View reviewed changes

hedera-mirror-rest/middleware/responseHandler.js Outdated Show resolved Hide resolved

hedera-mirror-rest/transactions.js Outdated Show resolved Hide resolved

hedera-mirror-rest/transactions.js Outdated Show resolved Hide resolved

Addressing PR comments.

72470f2

Signed-off-by: mgoelswirlds <[email protected]>

mgoelswirlds commented Sep 17, 2024

View reviewed changes

mgoelswirlds requested a review from xin-hedera September 18, 2024 00:07

steven-sheehy requested changes Sep 18, 2024

View reviewed changes

Modifying tests to match the object format.

e7b1761

Signed-off-by: mgoelswirlds <[email protected]>

steven-sheehy modified the milestones: 0.114.0, 0.115.0 Sep 18, 2024

Merge remote-tracking branch 'origin/main' into 9174-reduce-cache-sch…

08ebb62

…edule-transactions

xin-hedera requested changes Sep 19, 2024

View reviewed changes

hedera-mirror-rest/transactions.js Outdated Show resolved Hide resolved

hedera-mirror-rest/transactions.js Show resolved Hide resolved

mgoelswirlds added 3 commits September 19, 2024 14:41

Addressing PR comments.

faa145e

Signed-off-by: mgoelswirlds <[email protected]>

Addressing PR comments.

840f1fb

Signed-off-by: mgoelswirlds <[email protected]>

Adding missing test coverage.

e0661c6

Signed-off-by: mgoelswirlds <[email protected]>

mgoelswirlds requested review from steven-sheehy and xin-hedera September 19, 2024 19:43

xin-hedera requested changes Sep 19, 2024

View reviewed changes

hedera-mirror-rest/transactions.js Outdated Show resolved Hide resolved

hedera-mirror-rest/transactions.js Outdated Show resolved Hide resolved

mgoelswirlds added 2 commits September 20, 2024 11:05

Adding missing test coverage.

1e11fc0

Signed-off-by: mgoelswirlds <[email protected]>

Adding spec tests for shorter max age.

4507742

Signed-off-by: mgoelswirlds <[email protected]>

xin-hedera requested changes Sep 20, 2024

View reviewed changes

hedera-mirror-rest/controllers/networkController.js Outdated Show resolved Hide resolved

Moving content type to constants.

45ab8c3

Signed-off-by: mgoelswirlds <[email protected]>

xin-hedera reviewed Sep 21, 2024

View reviewed changes

xin-hedera approved these changes Sep 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reducing cache-control for scheduled transactions. #9327

Reducing cache-control for scheduled transactions. #9327

mgoelswirlds commented Sep 11, 2024 •

edited

Loading

codecov bot commented Sep 11, 2024 •

edited

Loading

steven-sheehy Sep 15, 2024

mgoelswirlds Sep 17, 2024

mgoelswirlds commented Sep 16, 2024 •

edited

Loading

mgoelswirlds Sep 17, 2024

steven-sheehy Sep 18, 2024 •

edited

Loading

mgoelswirlds Sep 19, 2024

steven-sheehy Sep 18, 2024

steven-sheehy Sep 18, 2024

steven-sheehy Sep 18, 2024 •

edited

Loading

mgoelswirlds Sep 19, 2024 •

edited

Loading

steven-sheehy Sep 18, 2024

xin-hedera Sep 19, 2024 •

edited

Loading

steven-sheehy Sep 19, 2024

steven-sheehy Sep 18, 2024

mgoelswirlds Sep 18, 2024

steven-sheehy Sep 18, 2024

mgoelswirlds Sep 18, 2024

steven-sheehy Sep 18, 2024

steven-sheehy Sep 18, 2024

sonarcloud bot commented Sep 21, 2024

xin-hedera Sep 21, 2024

xin-hedera commented Sep 21, 2024

		function getTransactionsByIdOrHashCacheControlHeader(transactionsRows, isTransactionHash, scheduled) {
		if (scheduled \|\| isTransactionHash) {

	"Content-type": "application/json; charset=utf-8"}
	"content-type": "application/json; charset=utf-8"}

		if (transaction.type === scheduleCreateProtoId) {
		if (TransactionResult.isSuccessful(transaction.result)) {

		if (successScheduleCreateTimestamp !== undefined
		&& (utils.nowInNs() - successScheduleCreateTimestamp) < maxTransactionConsensusTimestampRangeNs ) {

		const isTransactionHash = isValidTransactionHash(transactionIdOrHash);
		let scheduledParamExists = false;

Reducing cache-control for scheduled transactions. #9327

Are you sure you want to change the base?

Reducing cache-control for scheduled transactions. #9327

Conversation

mgoelswirlds commented Sep 11, 2024 • edited Loading

codecov bot commented Sep 11, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgoelswirlds commented Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

steven-sheehy Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steven-sheehy Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

mgoelswirlds Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xin-hedera Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 21, 2024

Quality Gate passed

Choose a reason for hiding this comment

xin-hedera commented Sep 21, 2024

mgoelswirlds commented Sep 11, 2024 •

edited

Loading

codecov bot commented Sep 11, 2024 •

edited

Loading

mgoelswirlds commented Sep 16, 2024 •

edited

Loading

steven-sheehy Sep 18, 2024 •

edited

Loading

steven-sheehy Sep 18, 2024 •

edited

Loading

mgoelswirlds Sep 19, 2024 •

edited

Loading

xin-hedera Sep 19, 2024 •

edited

Loading