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

Is/routes admin api #406

Merged
merged 31 commits into from Nov 13, 2019
Merged

Is/routes admin api #406

merged 31 commits into from Nov 13, 2019

Conversation

theotherian
Copy link
Collaborator

@theotherian theotherian commented Nov 9, 2019

Still a few things left to work on, but I feel like I've been working on this so long I need to get some eyes on it.

To do:

  • Add pagination to calls for collections of routes
  • Add more JavaDoc
  • Figure out proper boxed type when fetching all routes (probably shouldn't just load it all as instances of StaticRoute even if it's convenient)
  • Add better handling of error cases in controller with Problems framework
  • Expand controller test to ignore unknown fields? AccountSettingsSpringBootTest does this currently.

Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>
…ctor into is/routes-admin-api

Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>

# Conflicts:
#	connector-persistence/pom.xml
#	connector-persistence/src/main/resources/db/changelogs/base/changelog.xml
Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>
Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>
Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>
Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>
Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>
Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>
Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>
Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>
@codecov
Copy link

codecov bot commented Nov 9, 2019

Codecov Report

Merging #406 into master will increase coverage by 2.51%.
The diff coverage is 64.6%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #406      +/-   ##
============================================
+ Coverage     58.32%   60.84%   +2.51%     
- Complexity      741      804      +63     
============================================
  Files           236      247      +11     
  Lines          4533     4663     +130     
  Branches        173      179       +6     
============================================
+ Hits           2644     2837     +193     
+ Misses         1824     1755      -69     
- Partials         65       71       +6
Impacted Files Coverage Δ Complexity Δ
...dger/connector/settings/GlobalRoutingSettings.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...ector/server/spring/controllers/PathConstants.java 0% <ø> (ø) 0 <0> (ø) ⬇️
.../java/org/interledger/connector/routing/Route.java 0% <ø> (ø) 0 <0> (ø) ⬇️
.../properties/ConnectorSettingsFromPropertyFile.java 86.56% <ø> (-0.76%) 15 <0> (ø)
.../connector/routing/StaticRouteNotFoundProblem.java 0% <0%> (ø) 0 <0> (?)
.../connector/routing/NoOpExternalRoutingService.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...rg/interledger/connector/routing/RoutingTable.java 0% <0%> (ø) 0 <0> (?)
...g/interledger/connector/DefaultILPv4Connector.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...org/interledger/connector/routing/StaticRoute.java 0% <0%> (ø) 0 <0> (?)
...ector/routing/StaticRouteUnprocessableProblem.java 0% <0%> (ø) 0 <0> (?)
... and 36 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db0592c...6495cfc. Read the comment docs.

Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>
@theotherian
Copy link
Collaborator Author

A few REST design questions spring to mind on this:

  • I'm a subscriber to the philosophy that a GET on a collection resource that has no entities should just return a 200 with an empty collection rather than a 404. This is due to the fact that it's usually more arduous for the client to handle errors that don't represent a condition you're likely to fail on, and because I think it's a reasonable argument to say "there is an entity at this resource: it's an empty collection."
  • While DELETE is guaranteed to be idempotent, you can communicate this two ways: either it always succeeds with some 2XX or only succeeds when something is deleted and returns a 404 otherwise. Usually I just do a 2XX but that's probably out of laziness. While you could argue that a 404 when missing is more specific, you also have the case that another client deleted it before you anyhow so it may not offer much helpful detail and may also make the job of the client more difficult.
  • I'm opting to fail specifically on a constraint violation on the PUT rather than doing a prefetch. This is for 2 reasons: 1) a prefetch is vulnerable to race conditions and 2) in my case the PUT tolerates both create and update, meaning it's inaccurate to fail based merely on presence.

Curious if you have any strong feelings on any of these to the contrary.

Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>
@sappenin
Copy link
Collaborator

@theotherian apologies if some of my comments are redundant - I'm now seeing you thought of most of them in your PR comments.

theotherian and others added 12 commits November 11, 2019 11:24
…routing/StaticRouteAlreadyExistsProblem.java

Co-Authored-By: David Fuelling <sappenin@gmail.com>
Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>
Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>
Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>
Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>
Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>
Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>


Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>
…ctor into is/routes-admin-api

Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>

# Conflicts:
#	connector-server/src/main/java/org/interledger/connector/server/spring/settings/SpringConnectorConfig.java
#	connector-server/src/main/java/org/interledger/connector/server/spring/settings/web/SecurityConfiguration.java
#	connector-service-impl/src/main/java/org/interledger/connector/config/CaffeineCacheConfig.java
Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>
Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>
theotherian and others added 5 commits November 11, 2019 17:30
…ctor into is/routes-admin-api

Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>

# Conflicts:
#	connector-server/src/main/resources/application-blast_dev.yml
Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>
Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>
…ctor into is/routes-admin-api

Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>

# Conflicts:
#	connector-it/src/main/java/org/interledger/connector/it/topologies/ilpoverhttp/TwoConnectorPeerBlastTopology.java
#	connector-server/src/main/resources/application-spsp_dev.yml
#	connector-server/src/test/resources/application-test.yml
sappenin
sappenin previously approved these changes Nov 13, 2019
Copy link
Collaborator

@sappenin sappenin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nits from my side. If you want to merge as-is, we can address them in follow-ups or feel free to discuss here.

@sappenin sappenin added this to In progress in v0.1.0 Release via automation Nov 13, 2019
@sappenin sappenin added this to the 11/22/19 Sprint milestone Nov 13, 2019
@sappenin sappenin moved this from In progress to Pull request open in v0.1.0 Release Nov 13, 2019
Signed-off-by: Ian Simpson <ian.g.simpson@gmail.com>
@sappenin
Copy link
Collaborator

@theotherian Great stuff in this PR, nice work!

@theotherian theotherian merged commit 16d3043 into master Nov 13, 2019
v0.1.0 Release automation moved this from Pull request open to Done Nov 13, 2019
@theotherian theotherian deleted the is/routes-admin-api branch November 13, 2019 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants