Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ledger_data returns null instead of empty list when all entries are filtered out #4392

Closed
mDuo13 opened this issue Jan 19, 2023 · 0 comments · Fixed by #4398
Closed

ledger_data returns null instead of empty list when all entries are filtered out #4392

mDuo13 opened this issue Jan 19, 2023 · 0 comments · Fixed by #4398
Assignees
Labels
API Change Bug Good First Issue Great issue for a new contributor

Comments

@mDuo13
Copy link
Collaborator

mDuo13 commented Jan 19, 2023

Issue Description

When you specify the type field to the ledger_data method, it is possible no objects of the specified type are found. (This can even occur if those types do exist in the ledger, but not in the section that the server iterated over while serving your request.) When this happens, the state field of the response has the value null instead of being an empty array ([]).

This is not terrible, but preferably the response format should maintain the same data type so that clients can handle it consistently. For example, in Python you should be able to do for entry in state which would work correctly if state is an empty list but would raise an exception if state is null(or None after parsing the JSON in Python).

Steps to Reproduce

Make a request such as this one on Mainnet or any other network with sufficiently large network:

{
  "command": "ledger_data",
  "type": "amendments",
  "binary": false
}

Note 1: Pagination currently works correctly here, so if you do it correctly you will eventually find a non-null response with the Amendments singleton object.

Note 2: I think it's not possible to get a null response without using type to filter, since even the genesis ledger has a nonzero number of ledger entries. Maybe you could also get an empty list by carefully crafting a marker that represents the end of the ledger data? Unclear, but it wouldn't happen under normal operation.

Expected Result

The state field of the result should be an empty array, [].

Actual Result

The result has "state": null instead:

{
  "result": {
    "ledger": {
      "accepted": true,
      "account_hash": "BB3084F4BDC4893319F092C4371A567D6542B46A5F3178731AF3F6027162C169",
      "close_flags": 0,
      "close_time": 727401202,
      "close_time_human": "2023-Jan-18 23:53:22.000000000 UTC",
      "close_time_resolution": 10,
      "closed": true,
      "hash": "5450D641CE2F78110E589564330E08927A57F6E3A9121AD0A0DED2D910640B99",
      "ledger_hash": "5450D641CE2F78110E589564330E08927A57F6E3A9121AD0A0DED2D910640B99",
      "ledger_index": "77209704",
      "parent_close_time": 727401201,
      "parent_hash": "3481AD1630634CD5E3F029D196E52EF17D443F1B1ABC844AE50362165DB96AC1",
      "seqNum": "77209704",
      "totalCoins": "99989145267752917",
      "total_coins": "99989145267752917",
      "transaction_hash": "6B2126D3674D40BE0A866E3188833C39DB919C53E99FF10CFB241A29E02C0CCE"
    },
    "ledger_hash": "5450D641CE2F78110E589564330E08927A57F6E3A9121AD0A0DED2D910640B99",
    "ledger_index": 77209704,
    "marker": "000124613EE5581AFFBA4C729A0AD1046CEEB9FDFAE4E2C8C0147212F274301D",
    "state": null,
    "validated": true,
    "warnings": [
      {
        "id": 1004,
        "message": "This is a reporting server.  The default behavior of a reporting server is to only return validated data. If you are looking for not yet validated data, include \"ledger_index : current\" in your request, which will cause this server to forward the request to a p2p node. If the forward is successful the response will include \"forwarded\" : \"true\""
      }
    ]
  },
  "status": "success",
  "type": "response"
}

Environment

Confirmed this happens on reporting and P2P servers running the latest version (1.9.4). Not sure if this also applies to Clio.

Is This a Breaking Change?

Technically, this could maybe break some clients (relevant XKCD) if they are checking specifically for null in this case. However, this seems very unlikely since an empty array should also trigger correct behavior for most code (it's a "falsy" type in most languages).

Additionally, documentation lists the state field as being an "Array" type only, so fixing this returns it to how the documentation has been stating (for a long time) it should work.

I think we should also update the published policy on non-breaking changes to clarify that we don't consider it a breaking change to fix a bug to match how the documentation says something is supposed to work.

@intelliot intelliot added the Good First Issue Great issue for a new contributor label Jan 19, 2023
@drlongle drlongle self-assigned this Jan 24, 2023
intelliot pushed a commit that referenced this issue Mar 30, 2023
Change `ledger_data` to return an empty list when all entries are
filtered out.

When the `type` field is specified for the `ledger_data` method, it is
possible that no objects of the specified type are found. This can even
occur if those objects exist, but not in the section that the server
checked while serving your request. Previously, the `state` field of the
response has the value `null`, instead of an empty array `[]`. By
changing this to an empty array, the response is the same data type so
that clients can handle it consistently.

For example, in Python, `for entry in state` should now work correctly.
It would raise an exception if `state` is `null` (or `None`). 

This could break client code that explicitly checks for null. However,
this fix aligns the response with the documentation, where the `state`
field is an array.

Fix #4392.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Change Bug Good First Issue Great issue for a new contributor
Projects
None yet
3 participants