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

Changes I made for the http-api-gateway component #290

Merged
merged 5 commits into from
Dec 9, 2022

Conversation

cap10morgan
Copy link
Contributor

@cap10morgan cap10morgan commented Dec 6, 2022

These are the changes I made to the API for the http-api-gateway component (rename coming shortly): https://github.com/fluree/http-api-server

Happy to change anything in here, just let me know.

Part of #267

Copy link
Contributor

@bplatz bplatz left a comment

Choose a reason for hiding this comment

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

I think this all looks good!

Copy link
Contributor

@zonotope zonotope left a comment

Choose a reason for hiding this comment

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

This looks fine to me, but I do have a minor suggestion that you can take or leave as you see fit.

(let [address (<! (alias->address conn ledger-alias))
_ (log/debug "Attempting to load ledger from" address)
attempt (<! (jld-ledger/load conn address))]
(when-not (util/exception? attempt)
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nitpick, but there could be an exception here from a different error other than the ledger not existing. In that case the issue would be swallowed and the user would assume the ledger doesn't exist when it does but it just has another problem.

I don't think it's a big deal for now, but maybe we could look for a specific exception to ignore, and pass everything else through.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, true. I'll investigate. And/or eliminate this fn entirely. :)

(log/debug "Loading ledger from" address)
(<! (jld-ledger/load conn address))))))

(defn load-if-exists
Copy link
Contributor

Choose a reason for hiding this comment

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

Are all these load* functions meant to be part of the public api? It's not clear to me when I would use load vs load-if-exists.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should just decide if we're going to swallow exceptions here or not and just do one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, there's definitely room for improvement here. Basically I needed a way to implement idempotent create. This was my "simplest possible thing that could possibly work" but I'd be happy to push it a little further in the direction of the ideal.

@cap10morgan
Copy link
Contributor Author

OK @bplatz, @zonotope, and @dpetran I added an exists? fn to the API (and supporting protocol stuff and some tests for i it too that even run and pass in CLJS). If this all looks OK I'll adapt the http-api-server/gateway to use it.

@bplatz
Copy link
Contributor

bplatz commented Dec 8, 2022

Sounds good! Although as it relates to API and JS compatibility the ? is a bit troublesome. I think you'll either have to rename the API for just those, or consider renaming it for everything so it is consistent.

@cap10morgan
Copy link
Contributor Author

Sounds good! Although as it relates to API and JS compatibility the ? is a bit troublesome. I think you'll either have to rename the API for just those, or consider renaming it for everything so it is consistent.

I was going to export it as jldExists in flureedb.cljs and flureenjs.cljs. Does that solve the (whole) problem?

@bplatz
Copy link
Contributor

bplatz commented Dec 8, 2022

We only did the jld prefix originally to decide how we were going to reconcile the existing API and the JLD-specific calls.

Since we ditched backwards compatibility, all of the jld prefixes should get removed as this is now the API.

We should probably have a ticket opened for that and get it into alpha 1 so we don't break API after release.

@cap10morgan
Copy link
Contributor Author

We only did the jld prefix originally to decide how we were going to reconcile the existing API and the JLD-specific calls.

Since we ditched backwards compatibility, all of the jld prefixes should get removed as this is now the API.

We should probably have a ticket opened for that and get it into alpha 1 so we don't break API after release.

Ah, sure. In that case it would just be named exists in those namespaces.

@cap10morgan
Copy link
Contributor Author

We only did the jld prefix originally to decide how we were going to reconcile the existing API and the JLD-specific calls.

Since we ditched backwards compatibility, all of the jld prefixes should get removed as this is now the API.

We should probably have a ticket opened for that and get it into alpha 1 so we don't break API after release.

Created a ticket here: #293

@cap10morgan cap10morgan merged commit 3c11395 into main Dec 9, 2022
@cap10morgan cap10morgan deleted the feature/http-api-changes branch December 9, 2022 21:25
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