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

Private proxy fix #1941

Merged
merged 1 commit into from
Feb 9, 2025
Merged

Conversation

dsblank
Copy link
Member

@dsblank dsblank commented Feb 8, 2025

This PR fixes a missing sanitize method in the PrivateProxyDb

@dsblank
Copy link
Member Author

dsblank commented Feb 8, 2025

@stevenyoungs I think we should not allow proxies to return None from the db.get_OBJECT() (or any method) as that hides bugs. It should be the case that sanitizing the object removes all inaccessible handles/references.

@Nick-Hall
Copy link
Member

Returning None also leaks the information that the object exists, which is another reason not to do so.

@Nick-Hall Nick-Hall added this to the v6.1 milestone Feb 8, 2025
@dsblank
Copy link
Member Author

dsblank commented Feb 8, 2025

I've paired down this PR to just fixing bugs in proxies.

@dsblank
Copy link
Member Author

dsblank commented Feb 8, 2025

The LivingProxyDb is going to require a rewrite. And I think there is an error when one proxy is on top of another.

I'll go ahead and turn off draft, and do a major rewrite when @stevenyoungs is done.

@dsblank dsblank marked this pull request as ready for review February 8, 2025 16:14
@dsblank
Copy link
Member Author

dsblank commented Feb 8, 2025

@Nick-Hall, this could go into 6.0 as it is just a regular bug fix.

@dsblank dsblank changed the title Private proxy fixes Private proxy fixe Feb 8, 2025
@dsblank
Copy link
Member Author

dsblank commented Feb 8, 2025

Originally reported here: https://gramps-project.org/bugs/view.php?id=12886

@dsblank dsblank changed the title Private proxy fixe Private proxy fix Feb 8, 2025
@dsblank
Copy link
Member Author

dsblank commented Feb 8, 2025

When we revise the proxies to make sure they work correctly (sanitize all, proper proxy layering, no need to check for None, etc) we also need to look at caching the results. When I exported 2 people, one person was sanitized 6 times, and the other 3 times. Sanitizing is an expensive process and there is no need to do it more than once.

[I would put this note in a github issue if we turned those on. I see issues as being for developers, not a replacement for mantis].

@stevenyoungs
Copy link
Contributor

The change looks good to me.

Here are some observations from looking at the code around his change.
The implementation of copy_citation_ref_list has the following:

            handle = citation.get_reference_handle()
            source = db.get_source_from_handle(handle)
            if source and not source.get_privacy():
                clean_obj.add_citation(citation_handle)
  1. there is no check on handle.
    Is there any way a citation can exist without a reference_handle?
    I know the standard UI forces a source to be selected when creating a citation.
  2. if source
    A nice example of a check that can be removed if Minimal code changes to support type hinting of db methods #1934 gets accepted. get_source_from_handle would have raised an error. Even today, it will in most scenarios.
  3. if we are prepared to use source.private rather than call source.get_privacy() it can be optimised to get the raw object
    I'm undecided on this.
            data = db.get_raw_source_from_handle(handle)
            if not source.private:
                clean_obj.add_citation(citation_handle)

@dsblank
Copy link
Member Author

dsblank commented Feb 9, 2025

I just made this function to work and look exactly like the others. I am re-writing private and living proxies completely so I wouldn't worry about the details of this sanitize function.

@Nick-Hall Nick-Hall modified the milestones: v6.1, v6.0 Feb 9, 2025
@Nick-Hall
Copy link
Member

Yes. Keep bug fixes simple.

@Nick-Hall Nick-Hall force-pushed the dsb/private-proxy-fixes branch from 109348c to e041e53 Compare February 9, 2025 21:41
@Nick-Hall Nick-Hall merged commit e041e53 into maintenance/gramps60 Feb 9, 2025
3 checks passed
@Nick-Hall Nick-Hall deleted the dsb/private-proxy-fixes branch February 9, 2025 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants