Skip to content
This repository was archived by the owner on Sep 14, 2024. It is now read-only.

Conversation

@Leptopoda
Copy link
Collaborator

@Leptopoda Leptopoda commented Jan 9, 2023

My null-safety migration introduced two small parsing errors. Instead of fixing them this started an endeavor on generating a lib for the networking stuff.
Also I started reworking the UI to use the material3 spec (and make it prettier in my eyes) but wanting to add more information in the RecipeList I noticed many things like create date, modified date ... (params of the RecipeShort) where not present. And who might have guessed reworking the entire backend is obviously the thing to do instead of just parsing three more json fields 😅


This is still a work in progress as some stuff is not done yet like:

  • let the lib handle the authentication
  • Images don't load in the recipe screen
  • There is no "All Recipes" category
  • no cache :(
  • user avatar not loading
  • search not working
  • can't create or safe recipe
  • can't start a Timer
  • generated lib can't propperly fetch the api version
  • image fetching is not handled through the lib yet and is a bit error prone

For now those are all and should be just a bit of api fixing.

Not planned for this PR are:

  • covering the api with tests

The api spec in its current form is not usable for us. It contains some syntax errors and could be more convenient but I've opened a PR for most of them nextcloud/cookbook#1419.
Sadly dart interprets values in it's own way and the serialization ib used (built_value) does not parse every attribute the right way. This can be seen with Strings where an empty String is read as null and the recipe_id is read as an int and won't be parsed into a String.

I've patched the copy of the api spec in this pr by changing the id to int and marking the necessary attributes as nullable. Those won't make it into the mentioned PR so we'll need to manually patch it for now. Although changing 3 lines in a yaml file and running the openapi-generator should still be easier than writing everything by hand :)


Also I could also host and maintain the lib in my own repo and publish it to pub.dev but I don't think there is any demand outside of this project.

@Leptopoda
Copy link
Collaborator Author

Although the build should be passing I think the CI should be disabled on draft PRs (that's what they're for)
Don't think we need to waste CI minutes for this

@Teifun2
Copy link
Owner

Teifun2 commented Jan 10, 2023

Wow this looks like a biig project, and could be super usefull.

my offline storage branch is now tottal outdated :D
https://github.com/Teifun2/nextcloud-cookbook-flutter/tree/feature/offline-storage

But maybe something similar to my idea there could still be used to integrate offline-storage with the openapi integration!

For the CI i am not sure how i can deactivate only draft in the github action. i looked at some examples but there was no definitve answer, if you know how to do it please go ahead!

@j-koreth
Copy link
Contributor

Hey @Leptopoda , would deleting recipes or editing images (at least support in the backend) be included in the PR. I publish the iOS version.

@Leptopoda
Copy link
Collaborator Author

I don't think so :(
This PR is just a 'rewrite' of the same functionality.

My goal is to make it work the same as before. You can see above what's still missing or making problems.

The problem is probably the same as before (there is no api endpoint for this and we'd need to dig deeper into webdav stuff)

@Leptopoda
Copy link
Collaborator Author

Leptopoda commented Jan 16, 2023

@Teifun2
Most of it is done now except the All category (although this should be trivial to implement).
I'd like to ask if you are ok with merging this without any cache implemented?

Currently every pageload will ping the api wich means loading times on every page load.
I think the caching is more related to the aforementioned offline mode and have thought about a few caching strategies like:

  1. Using the bloc:
    to cache every response into a DB (and maybe even into memory for faster pageloads).
    This could be implemented from scratch or using something like hydrated_bloc which uses HiveDB
  2. Using a DIO interceptor:
    In theory a dio interceptor has access to the entire response. This could be used to either load and hydrate the cache.
    This could be achieved with something similar to dio_cache_interceptor.

I've played a bit using the later with the memory and hive backend. It speeds p the app quite a bit although it wasn't 100% mature (things like clearing the entire cache is not implemented).

TLDR; the cache in itself is a big thing and will go hand in hand with the offline mode. As to not bloat this PR even more I'd do this separately :)

P.S. as the generated API also has a method for fetching the image we could also get rid of cached_network_image and let all requests and caching be handled in one central place.

EDIT: please also consider merging #172 so I can push my chages here

@Leptopoda
Copy link
Collaborator Author

Leptopoda commented Jan 18, 2023

Ok mostly all things are done and working.
as mentioned above there is no cache an literally every request goes out to NC. The app version decoding is still falining but I'll have a look later.

otherwise this is ready to be tested.
I only have an NC that supports real https and uses SSO so I've only tested the app password login method.
Self signed certs, http or using the normal password is still untested although I expect them to not have major issues.

After debugging the API/APPversion stuff I'll maybe setup a little docker compose and cover some basic stuff in tests to ensure it's working fine.

I've mentioned that I was originally working on a redesing using Material3 and after this PR I'll finish it up first before doing the offline stuff. I've rebased it to often now :)

EDIT: The current api spec does not allow easy decoding of the AppVersion. The dart generator implements anyOffor oneOff keys by using polymorphism i.e.

abstract class AppVersion {}

class ReleaseAppVersion extends AppVersion {}

class PreReleaseAppVersion extends AppVersion {}

now the generated dart code can't decide how to decode the AppVersion field and it will fail. For this to work a discriminator would need to be sent within the api (the response would have the data and a type field telling dart how to parse the object). Read more about the recommended use of discriminators in the OpenAPI dosumentation.

This will need server side changes and as the client currently doesn't rely on this information I don't see this as something critical for this PR. I also don't see this as a major breaking point of the API as it's only adding a few fields most clients won't even read (afaik no other client currently implements code generation).

Therefore I see this PR as done for now and mark it as ready for review.

@Leptopoda Leptopoda marked this pull request as ready for review January 19, 2023 16:47
@j-koreth
Copy link
Contributor

I'll try testing it sometime today or tomorrow.

@Leptopoda Leptopoda force-pushed the openapi branch 2 times, most recently from 6312dc0 to b00f489 Compare January 22, 2023 12:47
@Leptopoda
Copy link
Collaborator Author

Leptopoda commented Jan 23, 2023

Small update:
please do not merge this yet. Testing is still welcome.

I'm currently upstreaming my changes to the openapi spec. I hope theyll go through but maybe the named endpoints (operationId) will change see nextcloud/cookbook#1445 (comment)

It would be bad if we'd have to change them again so soon (not hard as they are mostly used in the data_repository.dart but why not wait).

I'll update this post with a link to the PR on the main repo once made

EDIT:
the PR we wanna have merged is nextcloud/cookbook#1461

@j-koreth
Copy link
Contributor

@Leptopoda

I tried it out on my instance and it seems like logging in is broken. I don't have an app password

Simulator Screen Shot - iPhone 14 Pro Max - 2023-01-29 at 16 19 10
Simulator Screen Shot - iPhone 14 Pro Max - 2023-01-29 at 16 19 20

@Leptopoda
Copy link
Collaborator Author

Leptopoda commented Jan 29, 2023

Thanks for giving it a try

I cleaned up the auth handling a bit and covered it with some unit tests 🙃

What's the response time of ypur server? This is clearly a timeout but I never hit a similar even when the network is bad and I get a 40ms ping.

I could relax the timeout a bit but might end up rewriting the auth flow from scratch (and implement the browser based login flow)

I must say that exams are coming and I might not have the time to look into it until end of februrary.

@Teifun2
Copy link
Owner

Teifun2 commented Jan 30, 2023

@Leptopoda & @j-koreth i can confirm the timeout. I had the same issue.
My server is not super fast, but generally also not slow.

Increasing the timeout itself is not working there seem to be other issues

I/flutter ( 6133): Transition { currentState: Instance of LoginState, event: LoginButtonPressed {serverURL: , username: , isAppPassword: false}, isSelfSignedCertificate: false, nextState: LoginFailure { error: type '_InternalLinkedHashMap<String, dynamic>' is not a subtype of type 'String' in type cast } }

I will check if i can do something about it.

@Teifun2
Copy link
Owner

Teifun2 commented Jan 30, 2023

Interesting 😄

The authenticate call without app password goes to
/ocs/v2.php/core/getapppassword

But i get the following response
{"folder":"/Recipes","update_interval":5,"print_image":true}

This seems to me not to be the correct endpoint

@Teifun2
Copy link
Owner

Teifun2 commented Jan 30, 2023

That was easy. A simple mistake during refactoring.

Logout seems to be broken too :) if i have some time i will investigate and fix what i find.

@Leptopoda
Copy link
Collaborator Author

Leptopoda commented Jan 30, 2023

Yeah I changed a lot as said
I'd need to force push before :)

The logout is also already fixed

Only thing is that the server is returning invalid json so strictly confirming to the api will break the app. I opened an issue over there but my php is to bad to make a fix myself.

I'll push it tomorrow as I would like to go over the changes a last time.

Also my PR regarding the api changes hasn't landed over in the main repo yet and I'd hate to change it again. So we would first need that to go through so please consider this as a draft.

Finally I'm not even sure the used generator is the best. There are others out there that can integrate into the dart build system (see build_runner) and they might be better suited than a standalone tool. I'm also in contact with the author of [nextcloud-neon[(https://github.com/provokateurin/nextcloud-neon) who joined the NC team to work on openapi stuff. Ideally I'd like to have the lib upstreamed there so we can use it for the login flow or user avatar handling but for now this is a bigger task.


Sadly the project will come to a halt for the next month :( But I'll continue once time is more available again.

I will probably post some screenshots or even a draft pr for my UI rework. I think it is coming along very well and will bring fresh live into the app.

@Leptopoda Leptopoda marked this pull request as draft February 5, 2023 10:37
@Leptopoda
Copy link
Collaborator Author

Marking as draft. As the api generation will probably be done in a different way.

I'm trying to make use of nextcloud-neon a dart package developed by an NC dev in thier free time.
This lib would enable us to use the nextcloud loginflow v2 (the browser based login flow).
Also @j-koreth this lib does support the NC webdav endpoints so we could implement picture upload :)
Although I still think the cookbook app should have an abstraction from it that we can use through a normal API endpoint.

Some related information can be found here: nextcloud/neon#189 and here nextcloud/cookbook#1461

@Leptopoda
Copy link
Collaborator Author

Small update ...

My exams are over and I've worked a lot on the openapi generator to make it usable for us. The changes are not yet merged but are hopefully soon.

The changes to the spec just been merged so at least that part os done.

@Leptopoda
Copy link
Collaborator Author

Leptopoda commented Apr 1, 2023

I've split this up into a few commits thus this now depends on #183

The authentication now stays the same as I plan implementing the nextcloud loginflow when tackling #126
while the nextcloud lib is going to be a bit different this PR brings the data structure in line with the lib.

Changing the lib should be fairly trivial once ready!

EDIT:
As I plan to migrate to the nextcloud lib in the long turn I'm maintaining the current lib in it's own repo as not to bloat this one.
Feel free to check it out. All API endpoints are covered in unit tests so we can be sure it isn't complete garbage :)

@Leptopoda Leptopoda marked this pull request as ready for review April 1, 2023 18:37
@Leptopoda Leptopoda mentioned this pull request Apr 1, 2023
9 tasks
@Teifun2
Copy link
Owner

Teifun2 commented Apr 3, 2023

@Leptopoda if i understand you correctly this is now also ready to be merged.

However i see that github has a lot of problems to properly visualize the changes by this PR alone. It still includes all changes in the list that have been merged with individual PR's.

I think it could help if you reopen this pr, so that we have a properly diff.

@Leptopoda
Copy link
Collaborator Author

A quick rebase did the trick :)

@Leptopoda
Copy link
Collaborator Author

Also @Teifun2 this API will probably break backwards compatibility with old api versions. This means that only nc cookbook version 0.10.1 will be supported thus also dropping support for the legacy OwnCloud version (if that is still a thing)

@Teifun2 Teifun2 self-requested a review April 4, 2023 07:37
@Teifun2
Copy link
Owner

Teifun2 commented Apr 4, 2023

@Leptopoda I think it is not realistic that we can properly support multiple different api versions properly.

As long as the api version call is made and shows some kind of API verison warning (Outdated api, please upgrade plugin) we are good.

@Leptopoda
Copy link
Collaborator Author

Good to hear that you are on the same page :)
We currently where supporting multiple version so I just wanted to mention the cahnge.

Also seeing that this fixes the recipe create issue you might want to publish a new version once this went through.

@Teifun2
Copy link
Owner

Teifun2 commented Apr 4, 2023

@Leptopoda Yes i remember that i tried to support mutliple versions, but this gets out of hand so fast. with trying to figure out what compatability looks like, i think it is not worth the effort.

Should i wait also for material changes before releasing?

@Leptopoda Leptopoda merged commit de0535d into Teifun2:develop Apr 4, 2023
@Leptopoda Leptopoda deleted the openapi branch April 4, 2023 10:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants