Add GetInvalidPaths method to FieldMask for better validation feedback (Golang) #1689

Activity

1. puellanivis commented on Jun 23, 2025

   

   puellanivis

   on Jun 23, 2025

   Last edited by puellanivis

   Collaborator

   More actions

   🤔 Does numValidPaths reorder the paths input? It seems it just does return i when it finds an invalid path.

   If it doesn’t reorder the paths, then I don’t think the logic of your proposed implementation would actually hold? In that, it could return additional paths that are valid, but since they’re after an invalid path, you end up reporting them as if they were invalid.

   P.S.: And if it does reorder the paths, then that would be quite the astonishment.
2. stapelberg commented on Jun 24, 2025

   

   stapelberg

   on Jun 24, 2025

   More actions

   (FWIW, this issue reads a lot like it was generated by an LLM, so I won’t spend time on this until the author replies in their own human voice to @puellanivis’s question.)
3. s4s7 commented on Jun 25, 2025

   

   s4s7

   on Jun 25, 2025

   Last edited by s4s7

   Author

   More actions

   Thank you for your feedback!

   Here's the corrected implementation that properly checks each path individually.

   func (x *FieldMask) GetInvalidPaths(m proto.Message) []string {
       if x == nil {
           return nil
       }
       paths := x.GetPaths()
       invalidPaths := make([]string, 0, len(paths))
       for _, path := range paths {
           if numValidPaths(m, []string{path}) == 0 {
               invalidPaths = append(invalidPaths, path)
           }
       }
       return invalidPaths
   }

   However, this approach has performance implications due to nested loops and multiple descriptor retrievals.
   Here's a more efficient version.

   func (x *FieldMask) GetInvalidPaths(m proto.Message) []string {
   	if x == nil {
   		return nil
   	}
   	paths := x.GetPaths()
   	if len(paths) == 0 {
   		return nil
   	}
   
   	md0 := m.ProtoReflect().Descriptor()
           invalidPaths := make([]string, 0, len(paths))
   	for _, path := range paths {
   		md := md0
   		isValid := rangeFields(path, func(field string) bool {
   
                           // ----------------- same logic as numValidPaths -----------------
   
   			// Search the field within the message.
   			if md == nil {
   				return false // not within a message
   			}
   			fd := md.Fields().ByName(protoreflect.Name(field))
   			// The real field name of a group is the message name.
   			if fd == nil {
   				gd := md.Fields().ByName(protoreflect.Name(strings.ToLower(field)))
   				if gd != nil && gd.Kind() == protoreflect.GroupKind && string(gd.Message().Name()) == field {
   					fd = gd
   				}
   			} else if fd.Kind() == protoreflect.GroupKind && string(fd.Message().Name()) != field {
   				fd = nil
   			}
   			if fd == nil {
   				return false // message does not have this field
   			}
   			// Identify the next message to search within.
   			md = fd.Message() // may be nil
   			// Repeated fields are only allowed at the last position.
   			if fd.IsList() || fd.IsMap() {
   				md = nil
   			}
   			return true
   
                           // ------------------------------------------------------------------ 
   
   
   		})
   		if !isValid {
   			invalidPaths = append(invalidPaths, path)
   		}
   	}
   	return invalidPaths
   }

   I can certainly refactor and add test code.
   What do you think of this approach?