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

Move some dereferencing logic to hub-core and implement dereferencing from the ws modules (for sharing) #1721

Merged
merged 6 commits into from
Mar 10, 2022

Conversation

DyspC
Copy link

@DyspC DyspC commented Nov 23, 2021

Poke #1646

This should fix the dereferencing issues when sharing is enabled but I'm not satisfied with how I create and delete a scoped dereferencer each time the dereference method is called, but since the dereferencers are referenced from a singleton through static calls to Library I don't think there's another way to have access to the user from the fetchUriContent method.

WDYT?

Perhaps a cleaner and simpler way would be to fetch the specification only if it has a valid DOCUMENTATION row in the sharing table along with a message in the modal like this one. This removes the need for a new db version.

image

@jsenko jsenko self-requested a review November 29, 2021 13:04
@EricWittmann
Copy link
Member

I like this idea, at least as a stop-gap. This puts the burden on users to explicitly share any content. This way you can't publicly stealth-share someone else's content by referencing it from your own API and then sharing that. Instead you must create a public share of the referenced content as well. This makes sense.

@EricWittmann
Copy link
Member

@jsenko thanks for selecting this for review. Let me know if you can't find the time to review this!

@EricWittmann
Copy link
Member

@DyspC sorry for the delay. I'll try to get this reviewed ASAP.

@EricWittmann
Copy link
Member

OK I've finally had a chance to review this and think more about it. This is a tougher issue than I thought. Overall I think this approach is reasonable. But as you say, the global nature of the dereferencers in the Library is not great. We could fix that on the Library probably. And probably should either way.

The alternative idea that doesn't require DB changes is interesting. I was thinking we could validate the references when creating the link (fail to create the link if there are "private" references being used). That makes sense, but we also need to validate when doing the actual dereferencing, since the link sharing can be turned off on the references at any time.

I think overall I like the 2nd proposal better than this one, with the addition of the reference validation when link sharing is enabled. That means we also need better handling of the error we get when trying to resolve a reference that we cannot. The validation in the Editor I think is probably OK, but there are other places that I suspect will fail with poor error visibility:

  • Downloading the API
  • Project generation
  • Publishing to git

WDYT @DyspC ?

@DyspC
Copy link
Author

DyspC commented Dec 22, 2021

I also prefer the explicit share flow and indeed the dereferencer failing silently is an issue.

Although I was thinking about what could be shared a few days ago and it looks like only the designs where "Share documentation" was clicked end up in the sharing table, and we removed that option for AsyncAPI (and GraphQL probably) because the redoc template did not handle them.

I think using the sole documentation flag for this will increase pressure on this feature being available on all specification types. I remember fmvilas discussing this bit for AAI in #447

@EricWittmann
Copy link
Member

I think it's a good thing to put pressure on enabling Share Docs for all types. AsyncAPI already has something like ReDoc - the only reason it's not in use right now is that it didn't work last time I tried it. It's worth trying it again I think. As for GraphQL - we could look for an open source visualizer, or we could just vomit the text into a read-only Ace editor and call it a day (at least temporarily).

@DyspC
Copy link
Author

DyspC commented Feb 4, 2022

@EricWittmann I tried to get around the Library's silent failing in https://github.com/DyspC/apicurio-studio/pull/57/files (and reverting the DB stuff + an implementation for the second idea) if you'd like to take a look

@DyspC
Copy link
Author

DyspC commented Feb 22, 2022

Sorry to press on you like that @EricWittmann but I'm going on another mission around mid-March.
Can you take a look at the diff I referenced last time so we can wrap this up ?

@EricWittmann
Copy link
Member

EricWittmann commented Mar 8, 2022

Hey @DyspC - https://github.com/DyspC/apicurio-studio/pull/57/files looks really good to me. Let's get that merged ASAP!

Apologies it's taken us so long to look at this. Studio has been on the back burner more than I would like for a long time now.

However, good news: that might change soon!

@DyspC
Copy link
Author

DyspC commented Mar 8, 2022

Alright I'm done cleaning the branch
the builds will fail resolving dependencies until Apicurio/apicurio-data-models#409 is published

@EricWittmann
Copy link
Member

OK great I'll get a new release of apicurio-data-models done and then get this merged. Thanks @DyspC !!

@EricWittmann
Copy link
Member

Having some trouble with npmjs.com and 2FA. Working on it.

@EricWittmann EricWittmann merged commit 46f97b9 into Apicurio:master Mar 10, 2022
@EricWittmann
Copy link
Member

This PR has been MERGED!! :)

There was a unit test failing, and we had made some changes to the data-models project that made it harder to use as a java dependency. All fixed now. Took a few upstream releases (made harder because the npm publish is failing due to 2FA right now).

Thanks again @DyspC - sorry for the long road on this one. I appreciate your perseverance on it.

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.

2 participants