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

Allow users to publish with a generated key + Check minimum version fix #29

Closed
wants to merge 4 commits into from

Conversation

raucoule1u
Copy link

@raucoule1u raucoule1u commented Jun 29, 2017

In IPFS class, I added a method "publish" in the inner class Name to allow users to publish with a choosen key.
Furthermore, I marked the method "publish(Optional id, Multihash hash)" as Deprecated because the path is malformed : "name/publish?arg=id&arg=/ipfs/hash" (there are 2 parameters args) and does not comply with the HTTP API.

Since the version 0.4.10, the current algorithm doesn't work anymore because it compares two String objects so "3" > "10". I implemented a new one which compare two integers, recursively.

@raucoule1u raucoule1u changed the title Allow users to publish with a generated key Allow users to publish with a generated key + Check minimum version fix Jun 29, 2017
@@ -490,6 +492,28 @@ public Object log() throws IOException {
}
}

private boolean versionIsValid(String version) {
String[] parts = version.split("\\.");
Copy link

Choose a reason for hiding this comment

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

Please use proper semver lib. IMO it is much better idea.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This library needs some love and updating in general I will try and devote some time asap.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Kubuxu I prefer to avoid bringing in binary external dependencies for simple things like this. One major benefit of that approach is that it makes it easy to cross compile this library to JS (which I do elsewhere).

Copy link
Member

Choose a reason for hiding this comment

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

@ianopolous is there a reason you cross-compile this instead of using ipfs/js-ipfs-api?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @victorbjelkholm! All of our application logic is written in Java. Originally we had a parallel implementation in JS but it was so much slower to write correctly and error prone. Writing the original in Java and cross compiling lets us move much faster and with much more safety and guarantees. We do have a few interfaces where java code can call out to JS (or Java if it's not running in a browser), but we try to keep those to a minimum.

We are also extremely sensitive to importing random libraries, for security reasons. I have never built js-ipfs-api, but I would guess it brings in 100s of dependencies through npm. All of these would have to be audited in detail in our security audit.

Copy link
Member

Choose a reason for hiding this comment

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

@ianopolous I see. Your guess about the npm dependencies is indeed correct. Thanks for explaining it!

Copy link
Collaborator

Choose a reason for hiding this comment

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

@victorbjelkholm No worries! :-)

@raucoule1u
Copy link
Author

Does someone plan to do something ? The IPFS version 0.4.10 cannot be used with the API.

@Kubuxu
Copy link

Kubuxu commented Jul 7, 2017

@ianopolous is that good to merge?

@ianopolous
Copy link
Collaborator

I've just started working on a more general fix. In the meantime you can just use the previous version of IPFS (which was the latest 10 days ago)

@ianopolous
Copy link
Collaborator

I've pushed fixes for this, and added a few more methods, to master.

@Kubuxu
Copy link

Kubuxu commented Jul 7, 2017

Thanks @ianopolous

@Kubuxu Kubuxu closed this Jul 7, 2017
@raucoule1u
Copy link
Author

@ianopolous Thank you

@raucoule1u
Copy link
Author

raucoule1u commented Jul 10, 2017

@ianopolous By the way, could you create a new release and publish it onto the maven repo (which is currently https://jitpack.io/#ipfs/java-ipfs-api) ?

@ianopolous
Copy link
Collaborator

@raucoule1u done.

@raucoule1u
Copy link
Author

@ianopolous Thank you 😄

@raucoule1u
Copy link
Author

raucoule1u commented Jul 11, 2017

@ianopolous I cannot access the Multihash class, so the "cat" function cannot be used.
The dependency is present in pom.xml, so I should be able to access the class.

<dependency>
       <groupId>com.github.multiformats</groupId>
       <artifactId>java-multihash</artifactId>
       <version>v1.1.0</version>
</dependency>

It is the same for the MultiAddress class.

I got this warning :
[WARNING] The POM for com.github.ipfs:java-ipfs-api:jar:v1.1.0 is missing, no dependency information available

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants