ipfs-shipyard / java-ipfs-http-client

A Java implementation of the HTTP IPFS API
MIT License
538 stars 243 forks source link

The repo is super behind the node, did some updates to make it work again #193

Closed andrei-tara closed 1 year ago

autonome commented 2 years ago

@ianopolous are you up for reviewing this PR?

ianopolous commented 2 years ago

I'm seeing a lot of whitespace and linting changes, changing to your multiaddr implementation (is there a problem with the existing one?) and switching to Jackson for JSON, and using an external rest library. Possibly a bug in Multipart as well with a newline?

What problems are you trying to solve?

andrei-tara commented 2 years ago
ianopolous commented 2 years ago

Do you have an example of some JSON the parser fails on? I'd be very surprised to see this, as it's been used in diverse environments in production for over 15 years. I'd also be very surprised if json parsing was ever a bottleneck.

The reason we don't use jackson is to be more flexible and work in more environments. For example there are environments this library can be cross compiled to run in which don't support reflection. This means jackson won't work.

We can probably expand the http error handling here, but I'd rather not take on an external dependency for something so simple.

If there is a problem with the multiaddr library then we should fix that in the source library.

andrei-tara commented 2 years ago

Yes, just run the tests with latest IPFS implementation :)

ianopolous commented 2 years ago

I'm not seeing any JSON parsing errors with the latest kubo (v0.15.0).

andrei-tara commented 2 years ago

Maybe it is only on my side, but half of the APITest.java are failing because of the abovementioned problems.

5 min 48 sec
ianopolous commented 2 years ago

There are a bunch of failing tests yes, but none are due to JSON parsing.

ianopolous commented 2 years ago

I've updated the multiaddr library in master, which was the main problem. Though there are still a few failing tests.

andrei-tara commented 2 years ago

There are a bunch of failing tests yes, but none are due to JSON parsing.

If possible, please go to the reabit hole and try to fix the tests you will see what I mean

ianopolous commented 2 years ago

All the tests are now green on master except for 2 pubsub ones which is because the API has changed to be multipart.

There are no JSON issues.

andrei-tara commented 2 years ago

Just ran the tests with latest master code and they are still failing on my side :(

ianopolous commented 2 years ago

Which tests are failing apart from the two I mentioned (which are again nothing to do with JSON)?

ianopolous commented 2 years ago

I've updated the pubsub implementation now and all tests are green on master. Definitely no JSON errors. I've also improved the error handling. Are there any remaining issues you are aware of?

andrei-tara commented 2 years ago
image

this is how it looks on my side

ianopolous commented 2 years ago

How are you running the tests?

I've run them with "mvn test", "ant test", using IntelliJ and on the github runner, all of which are green.

These are all against kubo v0.15.0.

ianopolous commented 2 years ago

What command are you using to run kubo?

andrei-tara commented 2 years ago

IPFS is running inside a docker container, default configuration

ianopolous commented 2 years ago

What command is it running in docker to start kubo? Are you using the docker compose file in this repo? That's still on go-ipfs v0.6.0. I'll update it.

ianopolous commented 2 years ago

Ok I've done that, are you able to test it (I don't use docker)?

andrei-tara commented 2 years ago

Yes, docker compose image: ipfs/kubo:master-2022-09-01-d2c4927

ianopolous commented 2 years ago

To run the tests you need a custom run command which enables the experimental pubsub. Like this: https://github.com/ipfs-shipyard/java-ipfs-http-client/blob/master/docker-compose.yml

kevodwyer commented 1 year ago

closing as out-of-date