[explorer/nodewatch] task: add new route #343

AnthonyLaw · 2023-03-28T03:42:57Z

What was the issue?

to replace the statistics service, node watch is missing some endpoints, used in wallet and explorer.

What's the fix?

add a few properties in the nodedescriptor: isHealthy, isHttpsEnable, isWssEnable, restVersion
added a new endpoint

/api/symbol/nodes (query params ssl, limit, order)
/api/symbol/nodes/mainPublicKey/<main_public_key>
/api/symbol/nodes/nodePublicKey/<node_public_key>

codecov · 2023-03-28T16:45:30Z

Codecov Report

Merging #343 (1310d05) into dev (e08fb73) will decrease coverage by 0.02%.
The diff coverage is 98.80%.

❗ Current head 1310d05 differs from pull request most recent head 3f743cd. Consider uploading reports for the commit 3f743cd to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #343      +/-   ##
==========================================
- Coverage   98.91%   98.89%   -0.02%     
==========================================
  Files          54       54              
  Lines        1850     1909      +59     
  Branches       72       72              
==========================================
+ Hits         1830     1888      +58     
- Misses         20       21       +1

Flag	Coverage Δ
explorer-nodewatch	`98.38% <98.80%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
explorer/nodewatch/nodewatch/__init__.py	`96.03% <96.55%> (+0.03%)`	⬆️
explorer/nodewatch/nodewatch/NetworkRepository.py	`100.00% <100.00%> (ø)`
explorer/nodewatch/nodewatch/RoutesFacade.py	`96.74% <100.00%> (+0.66%)`	⬆️

Jaguar0625 · 2023-03-30T13:10:21Z

explorer/nodewatch/nodewatch/NetworkRepository.py

+		is_https_enable=None,
+		is_wss_enable=None,


enable => enabled (x2)

do we actually need BOTH of these?

For the explorer, we will need the is_https_enable, for the wallet it needs is_wss_enable.

Can we consider if https enable, we can assume wss is enable?

Can we consider if https enable, we can assume wss is enable?

yes, i would.
cc: @gimre-xymcity

explorer/nodewatch/nodewatch/RoutesFacade.py

explorer/nodewatch/nodewatch/NetworkRepository.py

explorer/nodewatch/tests/test_RoutesFacade.py

Jaguar0625 · 2023-03-30T15:34:56Z

explorer/nodewatch/tests/test_app.py

 	] == list(map(lambda descriptor: descriptor['name'], response_json))


+def test_get_api_symbol_nodes(client):  # pylint: disable=redefined-outer-name
+	# Act:
+	response = client.get('/api/symbol/nodes')


do we need a new /nodes route in addition to nodes/peer and nodes/api?

I was thinking about this as well, but not sure it's a good idea, here is my thought.

purpose

the endpoint mainly returns all nodes and displays them on explorer.

this endpoint added SSL, limit, and order filters, which will use to init explorer, and wallet to pull all healthy nodes.

calling 1 request instead of 2 to get all node lists.

But I didn't think that is the problem called 2 requests to get the node list, I can apply query params in the 2 endpoints as well.

what will you recommand?

I can apply query params in the 2 endpoints as well

I think I would prefer this approach, but also ask @gimre-xymcity his opinion.

explorer/nodewatch/tests/test_app.py

explorer/nodewatch/nodewatch/__init__.py

Jaguar0625 · 2023-04-11T13:15:44Z

explorer/nodewatch/tests/test_RoutesFacade.py

+			[2, 3],
+			list(map(lambda descriptor: descriptor['roles'], node_descriptors)))
+
+	def test_can_generate_nodes_json_filtered_order_random_limit_2(self):


also would rename to random_subset

Jaguar0625 · 2023-04-11T13:17:16Z

explorer/nodewatch/tests/test_RoutesFacade.py

@@ -361,15 +371,15 @@ def test_can_generate_nodes_json(self):
 		facade.reload_all(Path('tests/resources'), True)

 		# Act:
-		node_descriptors = facade.json_nodes(1)
+		node_descriptors = facade.json_nodes(role=1)


based on naming of test, this test should probably have no filter

Jaguar0625 · 2023-04-11T13:17:45Z

explorer/nodewatch/tests/test_RoutesFacade.py

 			list(map(lambda descriptor: descriptor['name'], node_descriptors)))
 		self.assertEqual(
-			[7, 3, 3, 5, 3],
+			[7, 3, 3, 5, 3, 3, 3],
 			list(map(lambda descriptor: descriptor['roles'], node_descriptors)))

 	def test_can_generate_nodes_json_filtered(self):


would add _by_role to this test and next

Jaguar0625 · 2023-04-11T13:18:57Z

explorer/nodewatch/tests/test_RoutesFacade.py

+		self.assertEqual(2, len(node_descriptors))
+		for name in random_node_names:
+			self.assertIn(name, full_node_names)
+


would add test with limit but without random

Jaguar0625 · 2023-04-11T13:22:17Z

explorer/nodewatch/tests/test_RoutesFacade.py

+		for name in random_node_names:
+			self.assertIn(name, full_node_names)
+
+	def test_can_generate_node_json_given_main_public_key(self):


consider test names:

can_find_known_node_by_main_public_key
can_find_known_node_by_node_public_key

cannot_find_unknown_node_by_main_public_key
cannot_find_unknown_node_by_node_public_key

Those names actually cause pylint error (name too long), but I'm ok with that name. so I used # pylint: disable=invalid-name

Jaguar0625 · 2023-04-13T21:34:28Z

explorer/nodewatch/nodewatch/RoutesFacade.py


 		def custom_filter(descriptor):
 			role_condition = True

 			if role is not None:
 				role_condition = role == descriptor.roles if exact_match else role == (role & descriptor.roles)

-			if ssl is not None:
+			if only_ssl is True:


do you need "is True" ?

Jaguar0625 · 2023-04-13T21:34:56Z

explorer/nodewatch/nodewatch/RoutesFacade.py

-			nodes = nodes[:limit]
-
-		return nodes
+		return nodes if limit == 0 else nodes[:limit]


if not limit. ?

https://github.com/symbol/product/blob/9a18900865ffe7c692eb0e796baf73da89446d46/explorer/nodewatch/nodewatch/__init__.py#LL107C3-L107C3

If that is not the limit, it will set it to 0.

Jaguar0625 · 2023-04-13T21:38:31Z

explorer/nodewatch/tests/test_app.py

 	full_api_node_names = [
 		'Allnodes250', 'Allnodes251'
 	]
 	actual_names = list(map(lambda descriptor: descriptor['name'], response_json))

+	# Assert:
 	_assert_symbol_node_response(response, actual_names)


assert len(actual_names) == 1

Jaguar0625 · 2023-04-13T21:39:13Z

explorer/nodewatch/tests/test_app.py

@@ -232,9 +233,9 @@ def test_get_api_symbol_nodes_peer_order_random_limit_2(client):  # pylint: disa
 		assert name in full_node_names


assert length of actual names is 2

Jaguar0625 · 2023-04-13T21:39:45Z

explorer/nodewatch/tests/test_RoutesFacade.py

@@ -226,7 +226,7 @@ def test_can_reload_all(self):
 		self.assertEqual(True, result)
 		self.assertEqual(facade.last_reload_time, facade.last_refresh_time)

-		self.assertEqual(6, len(facade.repository.node_descriptors))
+		self.assertEqual(9, len(facade.repository.node_descriptors))


consider adding a comment

Jaguar0625 · 2023-04-13T21:42:36Z

explorer/nodewatch/tests/test_RoutesFacade.py

+			[2, 2, 7, 3, 3],
+			list(map(lambda descriptor: descriptor['roles'], node_descriptors)))
+
+	def can_find_known_node_by_main_public_key(self):  # pylint: disable=invalid-name


test need to start with test_ prefix or they will not be detected as tests (x4)

Jaguar0625 · 2023-04-13T21:45:53Z

explorer/nodewatch/nodewatch/RoutesFacade.py

-			nodes = nodes[:limit]
-
-		return nodes
+		return nodes if limit == 0 else nodes[:limit]

 	def json_node(self, filter_field, public_key):


you might want to consider checking if public_key is proper public key and returning 400 if not

AnthonyLaw force-pushed the task/new-route branch 4 times, most recently from 14473d1 to d655a86 Compare March 28, 2023 08:51

AnthonyLaw requested review from Jaguar0625, 0x6861746366574 and gimre-xymcity March 28, 2023 16:58

AnthonyLaw force-pushed the task/new-route branch 2 times, most recently from 3f743cd to 1310d05 Compare March 30, 2023 01:31

AnthonyLaw added 5 commits March 30, 2023 09:36

[explorer/nodewatch] task: add new filed on NodeDescriptor and unit test

eadd734

[explorer/nodewatch] task: update unit test

574bc4d

[explorer/nodewatch] task: add few new api symbol endpoint

5052074

[explorer/nodewatch] task: fix lint issue

3442a05

[explorer/nodewatch] task: update unit test

1832550

AnthonyLaw force-pushed the task/new-route branch from 1310d05 to 1832550 Compare March 30, 2023 01:36

Jaguar0625 requested changes Mar 30, 2023

View reviewed changes

AnthonyLaw added 3 commits March 31, 2023 18:34

[explorer/nodewatch] task: rename enable to enabled

6bd5ee5

[explorer/nodewatch] task: rename and update filter logic

1e2628f

[explorer/nodewatch] task: and more unit test and minor refactor

9688016

AnthonyLaw requested a review from Jaguar0625 April 1, 2023 07:19

[explorer/nodewatch] task: refactor and unit test

214790a

AnthonyLaw force-pushed the task/new-route branch from 42f0d51 to 214790a Compare April 10, 2023 16:42

Jaguar0625 requested changes Apr 11, 2023

View reviewed changes

[explorer/nodewatch] task: more refactor and unit test

9a18900

AnthonyLaw requested a review from Jaguar0625 April 13, 2023 20:13

Jaguar0625 reviewed Apr 13, 2023

View reviewed changes

Jaguar0625 force-pushed the dev branch from abe44a3 to 6afff08 Compare May 16, 2023 17:53

AnthonyLaw added 2 commits May 26, 2023 04:27

[explorer/nodewatch] task: improve unit test

34721cb

[explorer/nodewatch] task: fix undetected unit test

3010a0c

[explorer/nodewatch] task: add validate public key

ab1851b

AnthonyLaw requested a review from Jaguar0625 May 25, 2023 20:48

Wayonb force-pushed the dev branch from 0f4774d to 6955c5e Compare June 15, 2023 02:02

Wayonb force-pushed the dev branch from 7a69984 to 964ad65 Compare July 12, 2023 01:53

Jaguar0625 force-pushed the dev branch 2 times, most recently from 901e815 to 2ee1d0e Compare July 14, 2023 16:58

Jaguar0625 force-pushed the dev branch from b736f95 to 45d803f Compare August 7, 2023 17:56

Wayonb force-pushed the dev branch from 45d803f to 0c1aa0c Compare August 7, 2023 22:38

Jaguar0625 force-pushed the dev branch from 7b5644c to 4fa2553 Compare August 17, 2023 14:28

gimre-xymcity force-pushed the dev branch from 767f4d2 to 5a9994b Compare August 18, 2023 11:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[explorer/nodewatch] task: add new route #343

[explorer/nodewatch] task: add new route #343

AnthonyLaw commented Mar 28, 2023

codecov bot commented Mar 28, 2023 •

edited

Loading

Jaguar0625 Mar 30, 2023

Jaguar0625 Mar 30, 2023

AnthonyLaw Mar 31, 2023 •

edited

Loading

Jaguar0625 Apr 11, 2023

Jaguar0625 Mar 30, 2023

AnthonyLaw Mar 31, 2023

Jaguar0625 Apr 3, 2023

Jaguar0625 Apr 11, 2023

Jaguar0625 Apr 11, 2023

Jaguar0625 Apr 11, 2023

Jaguar0625 Apr 11, 2023

Jaguar0625 Apr 11, 2023

AnthonyLaw Apr 13, 2023

Jaguar0625 Apr 13, 2023

Jaguar0625 Apr 13, 2023

AnthonyLaw May 25, 2023

Jaguar0625 Apr 13, 2023

Jaguar0625 Apr 13, 2023

Jaguar0625 Apr 13, 2023

Jaguar0625 Apr 13, 2023

Jaguar0625 Apr 13, 2023

		@@ -232,9 +233,9 @@ def test_get_api_symbol_nodes_peer_order_random_limit_2(client): # pylint: disa
		assert name in full_node_names

[explorer/nodewatch] task: add new route #343

Are you sure you want to change the base?

[explorer/nodewatch] task: add new route #343

Conversation

AnthonyLaw commented Mar 28, 2023

What was the issue?

What's the fix?

codecov bot commented Mar 28, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnthonyLaw Mar 31, 2023 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 28, 2023 •

edited

Loading

AnthonyLaw Mar 31, 2023 •

edited

Loading