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

Prowlarr /search Sonarr /release end points #162

Open
rraymondgh opened this issue Jun 21, 2024 · 24 comments · Fixed by #163 · May be fixed by #166
Open

Prowlarr /search Sonarr /release end points #162

rraymondgh opened this issue Jun 21, 2024 · 24 comments · Fixed by #163 · May be fixed by #166

Comments

@rraymondgh
Copy link

I can't see either of these end points in the API.

@davidnewhall
Copy link
Contributor

Hello, I've added methods and structs for both of these api endpoints. Can you test the new code? Thanks!

@davidnewhall
Copy link
Contributor

Thanks for responding to that feature request. There are a few reasons I switched from starr to resty

  • end points that I'm using not being present, however this is not primary reason. Have also identified /indexer/bulk
  • a very minor point, would need to use githash versioning as don't want to use latest
  • taking IndexerResource as an example structure. I'm looking for a common definition across prowlarr, sonarr and radarr. I see the /indexer endpoint across the apps as being the same. The differences can be fully ignored (just not defined in the struct)

Taking third point - this then allows functional code that implements download capability that is 100% common across arrs. There is no need to do any type coalescence between radarr.IndexerOutput, sonarr.IndexerOutput and prowlarr.IndexerOutput. Plus no need to implement interfaces that use almost identical functions for each of the apps

I did experiment with generics before refactoring code to use resty I just could not get it to work. This reasoning is maybe flawed - I'm new to go, the type strict approach is great. Possibly I'm just not seeing how to deal with heterogeneous structs that have very large overlap in an effective way.
Yeah, dealing with structs that are similar but not identical is always a pain.

I could write a copy function that uses reflection to copy all values from one struct to another if the name and types match. So you'd do something like this:

p := prowlarr.New(&starr.Config{
	URL:    "http://192.168.3.2:9696/prowlarr/",
	APIKey: "abc",
})

s := sonarr.New(&starr.Config{
	URL:    "http://192.168.3.2:8989/sonarr/",
	APIKey: "xyz",
})

indexers, err := p.GetIndexers()
if err != nil {
	panic(err)
}

var indexer *sonarr.IndexerInput
starr.Copy(indexers[0], indexer)

_, err = s.AddIndexer(indexer)
if err != nil {
	panic(err)
}

As for points 1 and 2: The library is never finished. The starr apps are under constant development. I make releases along the way, but we're always adding new methods and structs, so yes, you'd have to use a hashref to keep the newest version. If we got all the feature you need in place, I'd be happy to cut a release you can lock yourself to.

Your thoughts are welcomed.

@rraymondgh
Copy link
Author

Yeah, dealing with structs that are similar but not identical is always a pain.

I could write a copy function that uses reflection to copy all values from one struct to another if the name and types match. So you'd do something like this:

p := prowlarr.New(&starr.Config{
	URL:    "http://192.168.3.2:9696/prowlarr/",
	APIKey: "abc",
})

s := sonarr.New(&starr.Config{
	URL:    "http://192.168.3.2:8989/sonarr/",
	APIKey: "xyz",
})

indexers, err := p.GetIndexers()
if err != nil {
	panic(err)
}

My initial thought is GetIndexers() could be passed a pointer to a struct which is used by json.Unmarshall(). This way calling code can control struct types. This idea is a little unbaked....

var indexer *sonarr.IndexerInput
starr.Copy(indexers[0], indexer)

I've seen there are other libraries that do this. I think it could work.

_, err = s.AddIndexer(indexer)
if err != nil {
panic(err)
}


As for points 1 and 2: The library is never finished. The starr apps are under constant development. I make releases along the way, but we're always adding new methods and structs, so yes, you'd have to use a hashref to keep the newest version. If we got all the feature you need in place, I'd be happy to cut a release you can lock yourself to.

That's what I thought - hence why I see them as very minor challenges.

I'll branch my PR branch and try to refactor again to use starr That way I can be much more specific on what is preventing adopting starr. Using resty has been simple to integrate. A bit perverse, have created a jupyter notebook that does a pretty decent job of generating go struct definitions from the openapi JSON. I'll update on this refactoring - hopefully pretty quickly.

@davidnewhall
Copy link
Contributor

being able to unmarshal into a custom struct is not straight forward with starr, but it's not impossible. Here's an example saving prowler indexers into the sonarr type:

func main() {
	p := prowlarr.New(&starr.Config{})
	var output sonarr.IndexerOutput

	req := starr.Request{URI: path.Join(prowlarr.APIver + "/indexer", fmt.Sprint(indexerID))}
	if err := s.GetInto(ctx, req, &output); err != nil {
		return nil, fmt.Errorf("api.Get(%s): %w", &req, err)
	}
}

Writing this example makes me want to expose all the bpConstants at the top of each method file.


I don't personally care for this, and think a copy method would be easier to maintain. I'm almost done writing something you can test.

@davidnewhall
Copy link
Contributor

davidnewhall commented Jul 5, 2024

I spent much longer than I wanted to trying to figure out how to copy into an initialized []interface{} using reflection. Uhg.
This is probably more reliable anyway. Check it out?

I'll add that this still isn't perfect, and I'm willing to make more api-method-specific copy methods if you think it'd be useful. What I mean by that is a method named CopyIndexer that can take in any indexerInput or IndexerOutput type, and output it into a new IndexerInput type specific to the app.

Things this current implementation can't directly account for are "zeroing" the ID, possibly tags, and settings these:

EnableAutomaticSearch:   false,
EnableInteractiveSearch: false,
EnableRss:               false,
DownloadClientID:        0,

EDIT: It can account for these now ^ example in the linked pull request description.

@davidnewhall
Copy link
Contributor

Created the CopyIndexer method to give some ideas. Looking forward to your feedback.

@rraymondgh
Copy link
Author

Created the CopyIndexer method to give some ideas. Looking forward to your feedback.

Thanks, I've forked PR #163. It appears you have missed the prowlarr /search POST endpoint. The equivalent of the Grab() function on sonarr /release end point.

I will want the radarr /release end point as well. Plus /indexer/bulk on sonarr and radarr

To get me going with casting I've coded this:

func Recast(a, b interface{}) error {
	js, err := json.Marshal(a)
	if err != nil {
		return err
	}
	return json.Unmarshal(js, b)
}

func RecastSlice[
	S prowlarr.IndexerOutput | radarr.IndexerOutput,
	D sonarr.IndexerOutput](
	src []*S, dest []*D) error {
	for i, idx := range src {
		dest[i] = &D{}
		err := Recast(idx, dest[i])
		if err != nil {
			return err
		}
	}

	return nil
}

As I'm refactoring this I'm getting optimistic I can do away with casting and use generics.

@davidnewhall
Copy link
Contributor

The contents of #164 and #166 are merged in the dn2_both branch. Hoping it has everything you could need. Lemme know!

@rraymondgh
Copy link
Author

The contents of #164 and #166 are merged in the dn2_both branch. Hoping it has everything you could need. Lemme know!

I've just published this https://github.com/rraymondgh/starr/tree/dn2_both_local branch. Which has fixups to structures to resolve json marshalling errors

  • indexerFlags is problematic as it has different data types across prowlarr / sonarr / radarr search & release enpoints. Have commented to prevent being able to convert between structures
  • have redefined the structures using my code generator to make sure they are consistent.
  • radarr Grab...() functions were not working. Have stripped back to just GUID and IndexerId. This is all that is needed and a couple of weeks ago I did find a reference to this in sonarr / radarr support or GitHub issues that these are the only two required attributes
  • I did a timing test between resty implementation and starr implementation. Both are pretty much identical (being slower that I would hope)
  • I'm not inclined to use orbit package. I think the recasting functions are best kept local to functional capability for now rather than being in an external dependency
  • why have you got type Grab struct ? I think these should always be identical to output struct. This is how they are defined in json API
  • the /indexer/bulk endpoints worked perfectly without me finding any issues :-)

Thoughts - happy to generate a PR to be able to merge these changes back into your branch

@davidnewhall
Copy link
Contributor

Thanks for the follow-up! I have some questions :)

  1. I'll look into the indexer flags more. We can't just remove outputs entirely. :( Can you tell me more about the problems?
  2. Why do you need omitempty on random fields? That's only used when marshaling and these are outputs not inputs.
  3. My Radarr would not work without all the inputs. It continually gave me errors until I had them all. How can I reproduce what you did?
  4. I honestly have no idea why I made a Grab struct other than I thought they were different. I see now they're not.

@rraymondgh
Copy link
Author

rraymondgh commented Jul 8, 2024

Thanks for the follow-up! I have some questions :)

  1. I'll look into the indexer flags more. We can't just remove outputs entirely. :( Can you tell me more about the problems?

It's this code that fails:

func RecastReleaseSlice[
	S sonarr.Release | prowlarr.Search | radarr.Release,
	D prowlarr.Search](
	src []*S, dest []*D) error {
	for i, idx := range src {
		dest[i] = &D{}
		err := Recast(idx, dest[i])
		if err != nil {
			return err
		}
	}

	return nil
}

when converting from a radarr release

  1. Why do you need omitempty on random fields? That's only used when marshaling and these are outputs not inputs.

Whenever the json api states that it is nullable, you have omit empty. It's really to give JSON marshalling a better chance if the data types are slightly divergent between the arrs.

python code...

def js(p, nullable, dformat):
    omit = ",omitempty" if nullable else ""
    comment = f" // {dformat}" if dformat[0].isupper() else ""

    return f'`json:"{ p }{ omit }"`{comment}'

  1. My Radarr would not work without all the inputs. It continually gave me errors until I had them all. How can I reproduce what you did?

You have to call the GET endpoint before using the POST end point. Let me see if I can standalone go code that demonstrates what I found.

  1. I honestly have no idea why I made a Grab struct other than I thought they were different. I see now they're not.

@davidnewhall
Copy link
Contributor

davidnewhall commented Jul 8, 2024

Whenever the json api states that it is nullable, you have omit empty.

This is only true for inputs. We are not using these structs as inputs afaik. As an example I put omitempty on the BulkIndexer input structs. Can you tell me more about the actual problems you experienced? It's hard for me to accept solutions without understanding what got us there.

EDIT2: I'd also like to point out that season and episode numbers are always int not int64. Nothing will ever have enough episodes to eclipse an int, and I've kept that rather consistent. I'm also not sure what benefit a slice of pointers to base types is. ie. why change []string to []*string?

EDIT3: After confirming the type for indexlerFlags in all 5 apps, I think my suggestion is to add omitempty to it and nil it out before recasting.

@davidnewhall
Copy link
Contributor

I'm a bit more confused about IndexerFlags. It exists on the Release type and that's not a type you're re-casting, so why are they a problem for you? Please share the code that lead you to commenting them out.

@davidnewhall
Copy link
Contributor

I updated the dn2_grab branch.

  • Fixed a couple types, added missing members to Radarr's Release type.
  • Removed the Grab types.
  • Gave indexerFlags an omitempty so you can zero and re-cast them.
  • Made the extra "inputs" to GrabRelease in Radarr optional.
    • So you can pass only the GUID and IndexerID.
    • I still don't think that works, but you can now do it.

@davidnewhall
Copy link
Contributor

@rraymondgh
Copy link
Author

I figured out the radarr problem. Will fix. https://github.com/Radarr/Radarr/blob/3c737c2c172fdc07e08c754fe03c8ab4afd0fde9/src/Radarr.Api.V3/Indexers/ReleaseController.cs#L77-L81

Have just pulled the updated branch - compiled and run my tests. Getting this error again:

WARN	servarr/download.go:123	api.Post(v3/release): invalid status code, 500 >= 300, Value can not be null. (Parameter 'release.MovieId')
WARN	servarr/download.go:123	api.Post(v3/release): invalid status code, 500 >= 300, Value can not be null. (Parameter 'release.MovieId')
W

@davidnewhall
Copy link
Contributor

davidnewhall commented Jul 8, 2024

I just fixed that and pushed an update. You can now use radarr.Grab(guid, indexer) and it should work 🤞

If you're not using orbit go ahead and switch to branch dn2_grab so I don't have to keep updating dn2_both.

@rraymondgh
Copy link
Author

I just fixed that and pushed an update. You can now use radarr.Grab(guid, indexer) and it should work 🤞

If you're not using orbit go ahead and switch to branch dn2_grab so I don't have to keep updating dn2_both.

I just ran 20 info hashes via bitmagnet graphql interface. 10 are not supposed to results in downloads as they do not exist in my sonarr or radarr instances. 10 should result in downloads (5 movies and 5 episodes). All went through

@davidnewhall
Copy link
Contributor

Went through correctly, or you're saying 10 that should not download, did?

@rraymondgh
Copy link
Author

Went through correctly, or you're saying 10 that should not download, did?

Went through correctly - the way I've designed test suite. Some expected positive tests and some expected negative tests.

Just reviewed prowlarr search, there is an option to remove Grab struct there too

@davidnewhall
Copy link
Contributor

Just reviewed prowlarr search, there is an option to remove Grab struct there too

done.

@davidnewhall
Copy link
Contributor

Let me know if we're good, or if you need anything else.

After we're done here, I'll add release endpoints to lidarr and readarr for consistency.

@davidnewhall
Copy link
Contributor

I still sort of think you should consider using the orbit library instead of maintaining those Recast functions yourself. Resetting tags and IDs seems prudent at least.

@rraymondgh
Copy link
Author

Somewhat of a delay - I'm actually working on a new services docker container that will use this which can then optionally be used by bitmagnet. bitmagnet doesn't want to merge due to a reasonable concern that an indexer being called many times is logically wrong to invoke a download in a client. Hopefully I shouldn't be too long before I'm ready to ship a first alpha version of this container that can be used by bitmagnet

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